On 19/08/2011 8:02 AM, Clemens Eisserer wrote:
Mario wrote:It looks good to me, and I don't expect any issues after this patch. The only thing is that you should check the settings for the editor, I think you still use a mix of tabs and spaces :) Hmm, I should really check those eclipse settings ^^ Also, public final static AppContext getAppContext() Doesn't seem to be thread safe by nature, I don't know if this is part of the issue you found, so maybe you should consider to synchronize this as well. Could be I got this wrong ... but isn't the idea behind getAppContext() that it can be called from any thread, and get the AppContext(if there is any) back?
Right. The appContext table is a synchronized map keyed on the current ThreadGroup. The first thread created in a ThreadGroup for a given AppContext will create the AppContext before any other threads can be created, hence this is thread-safe.
The change to use AtomicInteger seems okay - the main concern would be class initialization problems, but presumably that doesn't happen. Part of the missing picture here is how AppContexts get created and dispose()'d as it may be that the way AppContext is used you can't actually get concurrent modification of numAppContexts. But there's no way to discern that from the AppContext code so it would be safer to use the AtomicInteger.
Cheers, David Holmes
- Clemens
