RFR: JDK-6956385: URLConnection.getLastModified() leaks file handles for jar:file and file: URLs [v2]

[Jesse Glick]

On Fri, 19 May 2023 15:19:10 GMT, Daniel Fuchs <dfuchs at 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-Boss at 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