On Tue, 14 Oct 2025 10:37:36 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to fix a file
>> descriptor leak in `sun.net.www.protocol.file.FileURLConnection`?
>>
>> The bug affects all JDK versions since Java 8 through mainline JDK.
>> Furthermore, the issue isn't in any way related to "jar:" URLs or the JAR
>> resources caching involved in the `JarURLConnection`.
>>
>> `sun.net.www.protocol.file.FileURLConnection` is a `java.net.URLConnection`.
>> For this issue, the APIs on `URLConnection` that are of interest are
>> `connect()`, header related APIs (the getHeaderXXXX(), getLastModified() and
>> similar APIs) and `getInputStream()`.
>>
>> The existing implementation in `FileURLConnection` in its `connect()`
>> implementation does readability checks on the underlying `File` instance. If
>> the `File` is a directory then the readability check is done by verifying
>> that a `File.list()` call does not return null. On the other hand, if the
>> File is not a directory, then connect() constructs a temporary
>> `java.io.FileInputStream` for the File and lets the FileInputStream's
>> constructor implementation do the necessary readability checks. In either
>> case, if the readability checks fail, then an IOException is thrown from
>> this method and the FileURLConnection stays unconnected.
>>
>> One important detail of the implementation in `FileURLConnection.connect()`
>> is that the `FileInputStream` that it creates for the regular-file
>> readability check, it keeps it open when it returns from `connect()`.
>> `connect()` itself doesn't return any value, so this `FileInputStream` that
>> was created for readability check remains open without the application
>> having access to it, unless the application calls
>> `(File)URLConnection.getInputStream()`. The implementation of
>> `FileURLConnection.getInputStream()` returns this previously constructed
>> `InputStream` if `connect()` had previously suceeded with its readability
>> checks. The application, as usual, is then expected to `close()` that
>> `InputStream` that was returned from `URLConnection.getInputStream()`. This
>> is the "normal" case, where the application at some point calls the
>> `URLConnection.getInputStream()` and closes that stream.
>>
>> As noted previously, `URLConnection` has a few other APIs, for example the
>> APIs that provide header values. An example:
>>
>>
>> Path regularFile = Path.of("hello.txt");
>> URLConnection conn = regularFile.toUri().toURL().openConnection();
>> // either of the following header related APIs
>> long val = conn.getLastModified();
>> String val = conn.getHeaderField...
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Daniel's review, use a class level field for URLConnection
test/jdk/sun/net/www/protocol/file/FileURLConnStreamLeakTest.java line 76:
> 74: void afterEach() throws Exception {
> 75: this.conn = null;
> 76: Files.deleteIfExists(this.testFile);
Suggestion:
// the file should already have been deleted by the test method
this.conn = null;
Files.deleteIfExists(this.testFile);
test/jdk/sun/net/www/protocol/file/FileURLConnStreamLeakTest.java line 97:
> 95: // verify that the URLConnection isn't holding on to any file
> descriptors
> 96: // of this test file.
> 97: Files.delete(this.testFile); // must not fail
Should we assert that Files.exists(this.testFile) is false?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2429273075
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2429283854
