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 JEP 238: 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. (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 (?). (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. (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 ..."? 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. thanks, -Sherman