Thanks Alan, I agree with both comments and will makes the changes before I push.
-Chris. On 19 Mar 2013, at 19:57, Alan Bateman <alan.bate...@oracle.com> wrote: > On 19/03/2013 17:43, Chris Hegarty wrote: >> JarFileFactory has two Maps that it uses to implement caching of jar files. >> Access to these maps should always be done while holding the instance lock, >> as multiple threads can be simultaneously updating the maps. >> >> The close() method was added some time after the original implementation, >> and it looks like the locking was forgotten. Also, the locking strategy >> assumes that JarFileFactory is a singleton. It should be reworked to make it >> more robust. There is much more cleanup that can be done here, and I intend >> to do it as a separate bug, so as not to confuse the specifics of this issue. >> >> http://cr.openjdk.java.net/~chegar/8010282/webrev.00/webrev/ >> >> -Chris. > I assume JarURLConnection.factory can be final. > > For getCachedJarFile then you could use "assert Thread.holdsLock(instance)" > in preference to the comment. > > Otherwise looks okay to me. > > -Alan.
