Hi Sherman, Thanks for looking at this. Comments in-line below.
> On Nov 17, 2015, at 9:49 AM, Xueming Shen <xueming.s...@oracle.com> wrote: > > On 11/11/15 8:44 AM, Steve Drach wrote: >> Hi, >> >> Please review the new webrev that addresses the issues raised by Sherman and >> Alan in the last iteration. In particular: >> - fixed the race condition in isMultiRelease() and another one with the >> variables ‘version’ and ‘configured’ >> - changed the fragment for JarURLConnection runtime versioning from >> ‘rtversioned’ to ‘runtime’, and created documentation for it >> - used try with resources to open JarFile in all the tests >> >> Issue: >> <https://bugs.openjdk.java.net/browse/JDK-8132734>https://bugs.openjdk.java.net/browse/JDK-8132734 >> <https://bugs.openjdk.java.net/browse/JDK-8132734> >> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305 >> <https://bugs.openjdk.java.net/browse/JDK-8047305> >> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ >> <http://cr.openjdk.java.net/%7Epsandoz/multiversion-jar/jar-webrev/> >> >> > > Hi Steve, > > Seems like the "version" is now a little confused. More likely it’s my confusion. > > (1) getVersioned() says "or 0 if this jar file is processed as if it is an > unversioned or is > not a multi-release jar file". The implementation now returns 8 > (BASE_VERSION is > 8 now) if not a multi-release jar (?). Yep. It should return 0 if version <= BASE_VERSION > > (2) setVersioned() says "If the specified version is 0 then this is > configured to be an > unversioned jar file". The implementation seems treat anything < BASE_VERSION > as > such? Sure, technically it's still correct. Given the BASE_VERSION is a > private field, any > thing between 0 and BASE_VERSION becomes unspecified gray area. As you say, it’s technically correct. We expect people will set version to 0 if they want unversioned processing. BASE_VERSION is not something clients should be concerned with, or even know about. So, yes, they can set versions to something less than BASE_VERSION but it doesn’t really matter, we know what they mean even if they don’t ;-) It’s a bit of a loose interpretation of the spec that doesn’t cause any harm. It doesn’t seem as useful if we strictly enforce this by throw an IllegalArgumentException for example. > > (3) getRuntimeVersionedEntry() spec says "If the JarFile is a multi-release > jar file and is > configured to be processed as such, then ...". However the implementation > > if (RUNTIME_VERSION > BASE_VERSION && !name.startsWith(META_INF) && > isMultiRelease()) { > ... > } > > does not appears to check the logic "is configured to be …"? Yes, clearly an error in the spec. > > And > > 391 if (manifestRead) { > 392 return isMultiRelease; > 393 } > 394 synchronized (this) { > 395 // this needs to be set prior to getManifest() call to > prevent recursive loop > 396 manifestRead = true; > 397 try { > 398 Manifest man = getManifest(); > 399 isMultiRelease = (man != null) && > man.getMainAttributes().containsKey(MULTI_RELEASE); > 400 } catch (IOException x) { > 401 isMultiRelease = false; > 402 } > 403 } > 404 return isMultiRelease; > don't we have a race condition if #391/2 is outside the synchronized block? > for sample, one > thread is inside sync block and set the manifestRead = true, but before > isMultiRelease = true/false, > and the second thread gets to #391. You’re right. once upon a time, there was a third boolean to deal with the potential loop, but I took it out and tried to overload manifestRead with dual meaning. I need to put the third variable back in. This is a surprisingly complex method, hard to get right. > > thanks, > -Sherman >
