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
