RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Xueming Shen]

Hi Steve,

The change looks fine.

thanks,
Sherman

On 11/18/2015 01:27 PM, Steve Drach wrote:
> 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