On Fri, 19 May 2023 13:00:30 GMT, Jesse Glick <[email protected]> wrote:
>> src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
>> line 96:
>>
>>> 94: return connected;
>>> 95: }
>>> 96:
>>
>> I wonder if it would be better to add a
>>
>>
>> void closeInputStream() { ... } // (or void close() { ... }?)
>>
>>
>> that would simply close the input stream if it's not null...
>
> `close()` would be natural enough, and would encapsulate behavior a bit
> better.
>
> Should it then actually implement `Closeable`, permitting the implicit
> request in https://github.com/openjdk/jdk/pull/12871#discussion_r1198873512
> to be addressed? That would not technically be an API change, since the
> implementation class is not exported, though it could be a behavioral change
> in case anyone is checking `instanceof Closeable` (or `instanceof
> AutoCloseable`); and would be a first step along the way to JDK-8224095,
> which may or may not be appropriate for something labeled a bug fix.
This is a good question. I we made `FileURLConnection` `Closeable` or
`AutoCloseable`, and called `close()` in `JarURLConnection` when the underlying
connection is `(Auto)Closeable` then I believe it could warrant a CSR. On the
other hand IIRC the `file:` protocol handler cannot be overridden - so I'm not
sure whether we could actually have a custom underlying `URLConnection` - that
would need to be carefully evaluated.
Also, as you note, instance of FileURLConnection may be returned to user code -
so the fact that the URLConnection implements `Closeable`/`Autocloseable` could
be observed, and would also require a CSR.
That said - we could simply expose a close() method on `FileURLConnection`
without implementing `Closeable`/`Autocloseable` - and could examine that
(implementing `Closeable`/`Autocloseable`) in a different PR - perhaps as a
Enhencement request - rather than a bug fix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12871#discussion_r1198989388
