RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

[Claes Redestad]

Hi Mandy,

On 03/15/2017 12:32 AM, Mandy Chung wrote:
> I agree with the goal to reduce the number of qualified exports, which I always like to keep.
>
> Duplicating code like this isn’t ideal although it’s straight-forward.  This is a performance optimization. One solution is to keep using the Manifest API and check the attribute value equals to `true` and separate the performance issue and explore any other solution.  Perhaps parsing of Manifest could be optimized.

yes, this enhances the performance of JarFileSystem::isMultiReleaseJar, 
which isn't strictly necessary
for the bug fix at hand[1], but based on that fact that the code in 
JarFile has been thoroughly tested
I figured this to be the lowest risk change rather than figuring out how 
to make the current
Manifest-reading code correct.

At a glance then making the existing code correct is likely trivial and 
minimal as you say, but I was
simply more comfortable copying/reusing code that I know works than 
delving into the details of an
implementation with which I'm less familiar.

If you insist and can review promptly I'll go ahead and make a minimal 
fix tonight, but I'd prefer to go
ahead with this well-known code before we hit RDP2 tomorrow.

Thanks!

/Claes

[1] While technically having JarFileSystem read the Manifest on creation 
*is* a performance regression
since that's never done in 8, I have no proof that this is in any way a 
critical regression.

>
> Mandy
>
>
>> On Mar 14, 2017, at 11:42 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
>>
>> Hi,
>>
>> Alan raised some concerns offline that we should try to reduce the
>> number of qualified exports, not adding more, and that it might be
>> better to accept some code duplication here. Thus I'm proposing this as
>> an alternative:
>>
>> http://cr.openjdk.java.net/~redestad/8176709/jdk.02/
>>
>> Neither solution is exactly pretty, but this approach removes any
>> performance risk of jdk.01 and by at least calling out that there's
>> some duplication around should avoid us slipping back into a similar
>> situation again.
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2017-03-14 16:04, Claes Redestad wrote:
>>> Hi,
>>>
>>> please review this change to adapt the JarFileSystem::isMultiReleaseJar
>>> method to align with the evolved specification in JEP 238
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176709
>>> Webrev: http://cr.openjdk.java.net/~redestad/8176709/jdk.01/
>>>
>>> This unfortunately adds a qualified export from java.base to jdk.zipfs,
>>> but since the jdk.internal.util.jar package was already exported to
>>> three other modules I think it's a low cost option that is preferable
>>> to other alternatives such as maintaining separate implementations.
>>>
>>> Thanks!
>>>
>>> /Claes