On Wed, 31 May 2023 09:29:49 GMT, Jaikiran Pai <j...@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
