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



<!--
        04/13/2007
        Email Signature File created by Houston Berry
        To be used by any Oracle employee with:
        Netscape 7+, Microsoft Outlook, Mac Mail 4.7+, or Thunderbird
        font-size used to set size in Mac Mail
//-->

<p> <br>
  <br>
  <a href="http://www.oracle.com" target="_blank"><img src="http://www.oracle.com/dm/design/images/oracle_sig_logo.gif" alt="Oracle" width="114" height="26" border="0"></a><br>
  <font color="#666666" size="2" face="Verdana, Arial, Helvetica, sans-serif"> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037<br>
  <font color="#FF0000">Oracle</font> Java Engineering <br>
  1 Network Drive <br>
  Burlington, MA 01803<br>
  <a href="mailto:Lance.Andersen@oracle.com">Lance.Andersen@oracle.com</a>
  </font></p>
<table width="100%" border="0" cellpadding="0" cellspacing="0">
  <tbody><tr>
    <td width="44" height="0" align="left" valign="top"><a href="http://www.oracle.com/commitment" target="_blank"><img src="http://www.oracle.com/dm/design/corp/misc/green-for-email-sig_0.gif" alt="Green Oracle" width="44" height="28" border="0" align="absbottom"></a></td>
    <td width="100%" align="left" valign="middle"><font color="#47AA42" size="1" face="Verdana, Arial, Helvetica, sans-serif"><font color="#4B7D42">Oracle is committed to developing practices and products that help protect the environment</font></font></td>
  </tr>
</tbody></table>
<p><br>
</p>



</div>
<br></div></body></html>