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

[Jaikiran Pai]

On Mon, 22 May 2023 18:01:15 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Ah! It's true only if the first time connect is called, it's implicitly by `initializeHeaders()` (sigh)...
>
> @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.

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

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