Bug 206475 - Keyring access from any thread
Keyring access from any thread
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Options&Settings
7.1
All All
: P2 (vote)
: 7.2
Assigned To: Petr Hejl
issues@platform
: API, API_REVIEW_FAST, THREAD
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-16 12:13 UTC by Petr Hejl
Modified: 2012-01-07 16:21 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
the patch (30.59 KB, patch)
2011-12-16 12:13 UTC, Petr Hejl
Details | Diff
updated patch (32.33 KB, patch)
2011-12-19 14:37 UTC, Petr Hejl
Details | Diff
updated patch (37.44 KB, patch)
2011-12-19 22:12 UTC, Petr Hejl
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Petr Hejl 2011-12-16 12:13:42 UTC
Created attachment 114261 [details]
the patch

I think it might be useful to allow that as there are situations which are deadlock prone such as #195943 and others. People are usually designing custom solutions (see the patch).

I'm attaching the patch that illustrates the situations and makes Keyring more safe. I'm not sure this should be part of the Keyring perhaps a separated support class would do the job as well.
Comment 1 Jesse Glick 2011-12-16 21:51:10 UTC
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.
Comment 2 Petr Hejl 2011-12-19 14:34:57 UTC
(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.
Comment 3 Petr Hejl 2011-12-19 14:37:22 UTC
Created attachment 114305 [details]
updated patch

Making code more readable, apichanges, javadoc, passing Cancellable to ProgressUtils.
Comment 4 Petr Hejl 2011-12-19 14:42:48 UTC
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.
Comment 5 Jesse Glick 2011-12-19 15:29:29 UTC
[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).
Comment 6 Petr Hejl 2011-12-19 22:12:15 UTC
Created attachment 114329 [details]
updated patch

JG01-04 Fixed. Updated arch.xml.
Comment 7 Petr Hejl 2011-12-19 22:21:10 UTC
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.
Comment 8 Jesse Glick 2011-12-19 22:43:58 UTC
Thanks, looks ready to commit.
Comment 9 Jaroslav Tulach 2011-12-20 08:50:48 UTC
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.
Comment 10 Petr Hejl 2011-12-20 09:48:58 UTC
(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.
Comment 11 Jaroslav Tulach 2011-12-20 11:27:56 UTC
(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.
Comment 12 Jesse Glick 2011-12-20 15:35:55 UTC
(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.
Comment 13 Petr Hejl 2011-12-20 20:07:59 UTC
(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.
Comment 14 Petr Hejl 2012-01-04 13:43:48 UTC
Thanks for the review. I'll integrate the change on Friday.
Comment 15 Petr Hejl 2012-01-06 09:04:56 UTC
Integrated into web-main 628798fafc03.
Comment 16 Quality Engineering 2012-01-07 16:21:38 UTC
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


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo