On Mon, 6 Oct 2025 14:30:07 GMT, Daniel Fuchs <[email protected]> wrote:
>> src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
>> line 145:
>>
>>> 143: public String getHeaderField(String name) {
>>> 144: initializeHeaders();
>>> 145: return super.getHeaderField(name);
>>
>> Several header related method implementations in this class have been
>> updated to avoid the `super.XXXX()` calls. The `super.XXX()` implementation
>> was calling `getInputStream()` and ignoring/not using the returned
>> `InputStream`. That call to `getInputStream()` was a way to initiate a
>> readability check and if the File wasn't readable then it would return a
>> different result compared to when it was readable. The use of
>> `getInputStream()` has been replaced by a call to `isReadable()` which does
>> the same checks without leaving around a `InputStream` instance. Apart from
>> that change, the rest of the code that resided in the `super.XXX()`
>> implementation has been literally copy/pasted in these methods to avoid any
>> kind of behavioural change.
>
> The code duplication is a bit distateful. Have you thought about introducing
> a package protected method in s.n.www.URLConnection e.g.
> ensureConnectionEstablished() that would by default call getInputStream()?
> Then you could possibly just override this method in the
> s.n.www.FileURLConnection subclass, instead of having to override all the
> methods that call getInputStream() just for the sake or replacing
> getInputStream() with isReadable()?
>
> I haven't experimented with what I'm suggesting here, and maybe you already
> considered and rejected it.
Hello Daniel, when I started on this fix, I intended to keep the changes only
to the `sun.net.www.protocol.file.FileURLConnection` to reduce the chances of
unforeseen problems. So I hadn't given it a thought to update the
`sun.net.www.URLConnection` class. But what you suggest here is much more
cleaner and at the same time allows us to fix this issue in
`FileURLConnection`. So I've updated this PR to follow your suggestion.
I decided to name the new (internal) method as `ensureCanServeHeaders()` to be
more precise about what its role is. I can rename it to something else, if
necessary.
I verified that the tests pass and the original leak is addressed even after
these latest changes.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2409775501
