On Tue, 15 Sep 2020 15:33:51 GMT, Daniel Fuchs <dfu...@openjdk.org> 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
