Hi Leonid, The fix looks good to me.
Just one comment: should we use a StringBuilder with some meaningful text instead of a plane Object for the lock? Even better if that were a named (inner) class. All for logging purposes when dumping a stack trace. What's your opinion?
-- best regards, Anthony On 10/02/2013 06: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.
