Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 16:59:59 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > I am fine with this as well. I will pull over the change and just sanity > check it via mach5 and then will sponsor @jaikiran Please go ahead and integrate this and I can then sponsor (has to be done in that order) - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > As for this: > >> As long as the input stream close() method is idem potent this should be >> safe, and AFAICS that is the case for the two >> input stream subclasses that can be returned by ZipFile::getInputStream. > > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this InputStream instance? I think I understand what you meant. You were perhaps talking about the `JarFile.close` triggering the `Cleanable` to close this `InputStream` in addition to the `try-with-resources` already calling `close` on that stream. Like you rightly note, the implementation of those streams already handles that aspect correctly. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this `InputStream` instance? My bad - I hadn't realised closing the input stream would also remove it from the Cleanable resource set, so I thought it might be closed again when the jar file is closed. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:33:51 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > Hi Jaikiran, > > This is not an area I know too well - so I won't review formally, but the > proposed changes look reasonable to me. > Closing the stream from within `JarFile` after creating the manifest looks > innocuous and should release any resource > held by the stream earlier instead of waiting for the `JarFile` to be closed. > As long as the input stream `close()` > method is idem potent this should be safe, and AFAICS that is the case for > the two input stream subclasses that can be > returned by `ZipFile::getInputStream`. WRT to the claims in the JBS issue I > see that that was logged against Java 6: > there was no `Cleanable` at this time and it is possible that the internals > of ZipFile/JarFile were quite different. Thank you for the review Daniel. > WRT to the claims in the JBS issue I see that that was logged against Java 6: > there was no Cleanable at this time and > it is possible that the internals of ZipFile/JarFile were quite different. You are right. I hadn't paid attention to that detail. It's likely that it might have been behaving differently at that time. As for this: > As long as the input stream close() method is idem potent this should be > safe, and AFAICS that is the case for the two > input stream subclasses that can be returned by ZipFile::getInputStream. I'm curious, in the context of this change, why idempotency would be a necessity. Would there be a "double close" somehow on this `InputStream` instance? - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:29:44 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove "final"
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 428:
>
>> 426: try (final InputStream is =
>> super.getInputStream(manEntry)) {
>> 427: man = new Manifest(is, getName());
>> 428: }
>
> There is a cleaner so shouldn't have a leak, even if the JarFile is not
> explicitly closed.
> The noisy "final" can be dropped, otherwise looks good.
Thank you for the review Alan. I've updated this PR to remove the `final`. And
yes as you note, this doesn't really
leak. This change closes the InputStream earlier, as soon as it is done,
instead of waiting for the `Cleaner` to kick
in.
-
PR: https://git.openjdk.java.net/jdk/pull/186
