Looks workable if ugly; would probably need to go through API review.

I guess you have tested this with the master password dialog? One issue I see is that the showProgressDialogAndRun call does not pass a Cancellable.

(In reply to comment #1)
> Looks workable if ugly; would probably need to go through API review.
> 
> I guess you have tested this with the master password dialog? One issue I see
> is that the showProgressDialogAndRun call does not pass a Cancellable.
Yes it has been tested with the master password dialog and gnome keyring. The similar code has been in use since 7.0.1 in j2ee and php.

Created attachment 114305 [details]
updated patch

Making code more readable, apichanges, javadoc, passing Cancellable to ProgressUtils.

Please review. The existing clients (avoiding calls from EDT as specified in javadoc) shouldn't be affected by this patch.

Take your time. I don't plan to integrate it this year.

[JG01] If the caller is on EQ, is there any guarantee that Keyring.save(k, v) immediately (or soon) followed by Keyring.read(k) will return v? Not sure if anyone depends on such behavior but you might expect it to work. (Compare Preferences impls which are expected to cache pref nodes and flush asynch.) It looks like the use of a single-threaded RP will in fact guarantee this, just not obvious.


[JG02] You might need to do more cleanup in client code simplified by this patch. Check for unused imports and bundle keys (e.g. DatabaseConnection_PleaseWaitMessage in db seems to be orphaned by the current patch).


[JG03] Perhaps obvious, but <compatibility> section of apichanges.xml should advise existing callers to use direct calls rather than trying to avoid EQ manually. (Wrap in <p>.)

May also be good to use <class> link; and mention that SPI implementors may continue to assume that they will not be called directly from EQ.


[JG04] @Messages should be on the nearest enclosing named element, in this case read(String).

Created attachment 114329 [details]
updated patch

JG01-04 Fixed. Updated arch.xml.

I haven't found any other code to remove however I have found couple of usages which could lead to deadlock with current Keyring implementation imo.

Thanks, looks ready to commit.

Y01 Looking at Keyring.read I have a feeling it works just by accident in case of default keyring implementation (the swing one). If the call comes from EDT, the implementation is called on background thread KEYRING_ACCESS and tries to show a Swing UI. That cannot be done, as EDT is blocked. So this times out. Then a showProgressDialogAndRun is called and shows a modal dialog which starts new event queue and the request from the background KEYRING_ACCESS thread is finally processed. Imho, the Keyring.read & Swing keyring impl should cooperate - as soon as the swing impl wants to show a UI, the Keyring.read should give up on EDT and let the UI handle its request.

(In reply to comment #9)
> Y01 Looking at Keyring.read I have a feeling it works just by accident in case
> of default keyring implementation (the swing one). If the call comes from EDT,
> the implementation is called on background thread KEYRING_ACCESS and tries to
> show a Swing UI. That cannot be done, as EDT is blocked. So this times out.
> Then a showProgressDialogAndRun is called and shows a modal dialog which starts
> new event queue and the request from the background KEYRING_ACCESS thread is
> finally processed. Imho, the Keyring.read & Swing keyring impl should cooperate
> - as soon as the swing impl wants to show a UI, the Keyring.read should give up
> on EDT and let the UI handle its request.

I don't see anything accidental - you described one possible code path. BTW implementation is called on background if the call comes from any thread not just EDT. 

Otherwise Y01 does not seem to be related to this API change. What you actually want to do is API between Keyring and fallback Keyring implementation that would improve Keyring implementation. Should be separate issue imo.

(In reply to comment #10)
> (In reply to comment #9)
> Otherwise Y01 does not seem to be related to this API change. 

The SAFE_DELAY seems to be observable from outside and as such it is a bit visible in this API change. I am not sure if 70ms is enough, at least for KDE I'd use higher value. On the other hand, if called from EDT and with default Swing implementation, the code waits for SAFE_DELAY ms uselessly.

(In reply to comment #9)
> as soon as the swing impl wants to show a UI, the Keyring.read should give up
> on EDT and let the UI handle its request

This was what used to happen before. The result was deadlocks like that in bug #195943 (the fix for which is now being moved into the API). jrechtacek can comment on whether that was the only possible fix; technically it was not the master password dialog which was causing the deadlock, but a private lock on DatabaseConnection.

Bug #190185 is another example, fixed differently, where something was blocking on Keyring and the lock was preventing EQ from running.

(In reply to comment #11)
> On the other hand, if called from EDT and with default
> Swing implementation, the code waits for SAFE_DELAY ms uselessly.
Still does not seem to be something to be reviewed here. Generally the keyring API would have to now something about the current provider in use.

Do you want to implement something like this?
if ("org.netbeans.modules.keyring.fallback.FallbackProvider".equals(provider().getClass().getName())) {
    // don't perform 70 ms wait
}

Anyway it would be simpler if you could update the patch in a way you prefer and we can review and test it. I think this approach would work better/faster.

Thanks for the review. I'll integrate the change on Friday.

Integrated into web-main 628798fafc03.

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/628798fafc03
User: Petr Hejl <phejl@netbeans.org>
Log: #206475 Keyring access from any thread