On Fri, Nov 5, 2010 at 5:51 PM, Lance Andersen - Oracle <lance.ander...@oracle.com> wrote: > > Hi Remi (and team), > I made changes to SyncFactory and one other class for a similar error. Also > cleaned up a couple of other minor issues in these classes. > The webrev can be found at http://cr.openjdk.java.net/~lancea/6982530/ > Thank you for catching the error. > Regards > Lance
Lance, I had a few questions and comments: In JdbcRowSetResourceBundle.java - private static String fileName is unused, can this be removed? My only hesitation is that the class is Serializable, and although the field is a static, the specification <http://download.oracle.com/javase/6/docs/platform/serialization/spec/version.html#6678> lists "Deleting fields" as an incompatible change without qualifying whether this implies deleting static fields is serial version incompatible (I wouldn't think it should be incompatible as statics aren't serialized by default, but I'm not a serialization expert by any means). In SyncFactory.java - the following fields are unused, can these be removed? private static String default_provider private static Level rsLevel private static Object logSync private static java.io.PrintWriter logWriter In SyncFactory.initJNDIContext(), isn't there still a potential visibility problem with reads of static Context field ic because the write is done under the class' monitor (in setJNDIContext), while the read is not? This could lead to a race condition which would possibly allow duplicate initialization under concurrent calls. Also, the static boolean field lazyJNDICtxRefresh does not seem to have any visibility guarantees, leading to similar problems. To fix this, should initJNDIContext() be synchronized? In SyncFactory.java, this is more stylistic, but I'm curious why a volatile double-checked lock was chosen over the (cleaner IMHO) lazy initialize-on-demand idiom described in Java Concurrency in Practice [Brian Goetz et al., 2006 p348]. The tradeoff I see is a mutable static volatile field versus the overhead of an additional class file. I'm just curious as I've been cleaning up a lot of unsafe double-checked locks in some code at work and I'd like to understand the rationale better from the core-libs perspective. I'd envisaged something along these lines with the removal of the private static volatile SyncFactory syncFactory: /** * Returns the {...@code SyncFactory} singleton. * * @return the {...@code SyncFactory} instance */ public static SyncFactory getSyncFactory() { // This method uses the thread safe lazy initialization holder class // idiom [Brian Goetz et al. Java Concurrency in Practice, 2006 p348] return Singleton.syncFactory; } private static class Singleton { static final SyncFactory syncFactory = new SyncFactory(); } Thanks, Dave
