On Fri, 19 May 2023 15:19:10 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Jesse Glick has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Encapsulate logic as `FileURLConnection.closeInputStream` as suggested by >> @dfuch https://github.com/openjdk/jdk/pull/12871#discussion_r1198859517 >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> FileURLConnectionLeak-JDK-6956385 >> - Simplified `instanceof` form >> >> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - 6956385: `JarURLConnection` may fail to close its underlying >> `FileURLConnection` > > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > line 97: > >> 95: is.close(); >> 96: is = null; >> 97: connected = false; > > Not sure about switching `connected` to false or setting `is` to null. That > might allow to call `connect` again and might have unintended side effects? It depends on whether you consider the new method to be the opposite of `connect`, toggling between connected and unconnected states. Removing these two lines would mean that a subsequent call to `getInputStream()` would return a closed stream, which may or may not be surprising. An aspect of JDK-8224095 is that the state transitions for `URLConnection` are generally confusing. For now it seems irrelevant since `JarURLConnection.jarFileURLConnection` is never used for its input stream. In fact I think `closeInputStream` could just be moved into `JarURLConnection.<init>` for this reason. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12871#discussion_r1199101446
