On Mon, 22 May 2023 18:01:15 GMT, Daniel Fuchs <[email protected]> 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
