Ok, here an updated webrev then, with the test added: http://cr.openjdk.java.net/~leonidr/8019623/webrev.01/
On Oct 8, 2013, at 7:43 PM, Artem Ananiev <[email protected]> wrote: > > On 10/8/2013 6:54 PM, Leonid Romanov wrote: >> Hi, >> I tried different variations of your suggestion and none of them worked. The >> reason is timing: suppose two threads, A and B, entered >> AppContetx.getAppContext() and then AppContext.initMainAppContext() at the >> same time. We need thread A to be able to initialize mainAppContext, return >> from initMainAppContext() and then initialize local variable with AppContext >> to be returned without thread B intervening in between. This just doesn't >> happen. > > This is perfectly expected, that the created regression test won't fail > without your fix. It will still be useful to prevent regression in the future. > > Thanks, > > Artem > >> On Oct 4, 2013, at 1:06 PM, Artem Ananiev <[email protected]> wrote: >> >>> Hi, Leonid, >>> >>> the fix looks fine. >>> >>> I believe it is possible to create a regression test for this fix. The test >>> should create several thread groups and call AC.getAC() there, then check >>> that all the returned AC's are equal. Although it won't 100% fail before >>> your fix, it should 100% pass after it. What do you think about it? >>> >>> Thanks, >>> >>> Artem >>> >>> On 10/2/2013 6:22 PM, Leonid Romanov wrote: >>>> Hello, >>>> Please, review a fix for 8019623: Lack of synchronization in >>>> AppContext.getAppContext(). I don't think it makes sense to ensure that >>>> AppContext.getAppContext()/SunToolkit.createNewAppContext() is thread >>>> safe, because in practice we haven't had any problems with this >>>> combination. What we have had problems with is the >>>> getAppContext()/getAppContext() combination because of the double >>>> mainAppContext initialization. Therefore, the fix takes care of this >>>> issue. While I could have simply made the whole AppContext.getAppContext() >>>> method synchronized, I wanted the most common path through it (returning >>>> cached AppContext) to remain lock-free, so I decided to place the lock >>>> around the most critical part, where initMainAppContext() gets called. >>>> As for a regression test, I couldn't find a way to detect double >>>> initMainAppContext initialization. We put newly created AppContexts into a >>>> thread-safe map, so whatever AppContext wins the race remains in the map, >>>> and the other is simply lost, without any convenient means to detect its >>>> existence. >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8019623 >>>> Webrev: http://cr.openjdk.java.net/~leonidr/8019623/webrev.00/ >>>> >>>> Thanks, >>>> Leonid. >>>> >>
