Quite unusual to start a name of a class (or a variable) with a verb
("GetAppContextLock"), but otherwise the fix looks fine to me.
--
best regards,
Anthony
On 10/09/2013 04:32 PM, Leonid Romanov wrote:
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.
