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

[Steve Drach]

Hi Sherman,

Thanks for looking at this.  Comments in-line below.

> On Nov 17, 2015, at 9:49 AM, Xueming Shen <xueming.shen at 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
>