hg: jdk7/tl/jdk: 6843995: RowSet 1.1 updates

[Lance Andersen - Oracle]

Hi David,

Thanks for the feedback.

Please see below.

On Nov 6, 2010, at 2:04 AM, David Schlosnagle wrote:

> On Fri, Nov 5, 2010 at 5:51 PM, Lance Andersen - Oracle
> <Lance.Andersen at 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).

Static fields are not serialized and this field was also never used so it could be deleted.

I have additional clean up that needs done and I plan to use a different CR than the one for the synchronization issue.  So I will make this change in a different putback
> 
> 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

Yes they can go.   It looks like the original author  forgot to remove these fields when they ended up not getting used.  I will do this in a separate putback though.
> 
> 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?

Yes, initJNDIContext() should also be a synchronized method.  I have updated the webrev
> 
> 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();
>     }
> 

Good question.  I had considered changing it but decided against it.  However  you made me reflect on it again so I decided to.  Effective Java 2nd Edition, ITEM 71,  suggests that you should use this idom for static fields instead of DCL as it is slightly more performant.  It still suggest DCL for instance fields for performance.

I have pushed the new webrev out.

Regards,
Lance
> Thanks,
> Dave




Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com

	Oracle is committed to developing practices and products that help protect the environment


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20101106/17c73dca/attachment.html>