Hi,
Please review the latest iteration of the webrev for runtime support of
multi-release jar files.
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/
I believe I addressed all issues brought up in the last review as follows.
In JarURLConnection then it would be good if the reference to multi-release
JARs should link to the description in the JarFile spec.
I included a comment in JarURLConnection referencing JarFile for more
information regarding multi-release jar files.
In the previous round then we were discussing renaming the
jdk.util.jar.multirelease property. Has there been any more on that?
I changed the property to jdk.util.jar.enableMultiRelease={true | false |
force} as suggested. I assume camel case is okay here.
The test MultiReleaseJarURLConnection uses @library
/lib/testlibrary/java/util/jar so it's reaching across the file system to use
the infrastructure of the JarFile tests. It might be clearer to move the test
to the JarFile directory.
I’m not sure what it means to reach across the file system. The bulk of the
multi-release tests are in jdk/test/java/util/jar/JarFile with the
MultiReleaseJarURLConnection test in jdk/test/sun/net/www/protocol/jar. Both
test directories refer to the third directory,
jdk/test/lib/testlibrary/java/util/jar, for common resources. All three
directories are in the jdk repo. I don’t see anything obviously incorrect
about where things are, the tests are in the same hierarchy as the components
they test. So pending further information, I left it as it was.
It would be nice if we could reduce some of the really long lines if possible,
just to make future side-by-side a bit easier (avoid horizontal scrolling).
I put the right margin for code at 100 and wrapped lines exceeding that.
(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 (?).
I fixed it to return 0.
(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.
I clarified it in the documentation. It now says "If the specified version is
less than 9, then this JarFile is configured to be processed as if it is an
unversioned jar file.” I also made some additional clarifying documentation changes
in overall documentation for JarFile.
(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 …”?
I took the “is configured to be…” phrase out of the documentation.
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.
We rewrote the isMultirelease() method to eliminate the race. We also
simplified it, removing the recursive invocation, with a small change to the
getManEntry() method.
Thanks,
Steve
