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

[Daniel Fuchs]

On Wed, 31 May 2023 09:29:49 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> @Michael-Mc-Mahon what do you think?
>
> Hello Jesse, Daniel, I had a look at the existing code in `sun.net.www.protocol.file.FileURLConnection` its super class `sun.net.www.URLConnection` and then its superclass `java.net.URLConnection`.
> 
> The various header related methods in `sun.net.www.URLConnection` first call the `getInputStream()` (and ignore that stream completely) and then check the `properties` field to get the header value. The call to `getInputStream()` is an internal implementation detail (since the `java.net.URLConnection` public APIs make no mention that it's required or will be called) and it seems to me that it's being called to make sure that the underlying resource is "connectable/available". The returned `InputStream` itself never gets used.
> 
> For the `FileURLConnection` class, the `InputStream` plays no role to populate these header fields. Yet, before the changes in this PR, it does end up creating an `InputStream` when trying to populate these header fields. I think the cleanest fix would be to not create this `InputStream` when populating these header fields (i.e. from `initializeHeaders()` method). That would mean that the `initializeHeaders()` wouldn't need to call the `connect()` method, like what's being proposed in this discussion thread. I think not calling `connect()` from `initializeHeaders()` (which gets called from various getHeader... methods) will still satisfy the specification because `java.net.URLConnection#connect()` method states:
> 
>>
>>...
>> Operations that depend on being connected, like getContentLength, will implicitly perform the connection, if necessary.
> 
> The "if necessary" in that sentence I think allows us to not "connect()" when populating and returning header fields.
> 
> I think the only place we should call connect() internally in this `FileURLConnection` is when `getInputStream()` method gets called on this class.
> 
> P.S: The current implementation in `initializeHeaders()` has some oddities. For example, if the `File` was a directory then in `connect()` we set `isDirectory()` as `true`. Then at some later point, when someone calls a getHeader... method and `initializeHeaders()` gets called, if the directory was deleted from the filesystem, then `initializeHeaders()` `exists` will be false, so it will go into the `if (!initializedHeaders || !exists) {` block but will still end up using the old outdated `isDirectory` flag to populate the `properties`. That's a different unrelated issue though.

I believe we should wait for @Michael-Mc-Mahon input before reworking these methods to not call `connect()`. There seems to be an assumption that `connect()` would/should be called.
What I like with the current fix is that it's a very narrow fix which is unlikely to cause too much backward compatibility issues.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/12871#discussion_r1211437025