Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v5]
On Tue, 2 Apr 2024 16:29:21 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Implemented review comments. Thank you for the update, Bill. The latest changes look OK to me. I also heard from Bill that Alan has approved the changes too. Before integrating, please run tier1, tier2 and tier3 (I haven't checked which tiers these changed tests belong to) to make sure nothing breaks unexpectedly. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18352#pullrequestreview-1975307573
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v5]
> This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Implemented review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/18352/files - new: https://git.openjdk.org/jdk/pull/18352/files/0f4130a9..81244609 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18352=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18352=03-04 Stats: 30 lines in 2 files changed: 12 ins; 5 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v4]
On Tue, 26 Mar 2024 18:18:39 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update test/jdk/java/nio/channels/unixdomain/Bind.java
>
> Co-authored-by: Andrey Turbanov
For the changes in `test/jdk/java/nio/file/Files/CheckPermissions.java` it
would be good to have Alan and/or Brian's inputs. The test documentation says
that the test relates to SecurityManager checks (and not filesystem permission
checks), so the proposed changes in this PR to use a different parent directory
for the temporary file(s) should be OK, but I am not familiar with that area to
be certain about it.
test/jdk/sun/security/pkcs12/P12SecretKey.java line 69:
> 67:
> 68: File ksFile = File.createTempFile("test", ".test");
> 69: ksFile.deleteOnExit();
Is there a reason why this change is done differently than the rest of the
changes in this PR? Can we delete the `File` in a try/finally instead of
enrolling a shutdown hook to delete it?
-
PR Comment: https://git.openjdk.org/jdk/pull/18352#issuecomment-2032269381
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1548045308
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v4]
On Tue, 26 Mar 2024 18:18:39 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update test/jdk/java/nio/channels/unixdomain/Bind.java
>
> Co-authored-by: Andrey Turbanov
test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java line 68:
> 66: // or exception could be thrown otherwise
> 67: } finally {
> 68: Files.deleteIfExists(addr.getPath());
Hello Bill, `addr` can be `null` here, so this has a potential of a
`NullPointerException`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1548038473
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v4]
> This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/nio/channels/unixdomain/Bind.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/18352/files - new: https://git.openjdk.org/jdk/pull/18352/files/2517f756..0f4130a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18352=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18352=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed potential NPE issues. unixdomain NIO test changes look fine to me - Marked as reviewed by michaelm (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18352#pullrequestreview-1960889711
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed potential NPE issues.
test/jdk/java/nio/channels/unixdomain/Bind.java line 162:
> 160: // address with space should work
> 161: checkNormal(() -> {
> 162: UnixDomainSocketAddress usa =
> UnixDomainSocketAddress.of("with space");
Suggestion:
UnixDomainSocketAddress usa = UnixDomainSocketAddress.of("with
space");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1538701870
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
On Thu, 21 Mar 2024 17:13:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed potential NPE issues. The bug should have a `noreg-self` label. - PR Comment: https://git.openjdk.org/jdk/pull/18352#issuecomment-2013158192
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v3]
> This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Fixed potential NPE issues. - Changes: - all: https://git.openjdk.org/jdk/pull/18352/files - new: https://git.openjdk.org/jdk/pull/18352/files/620f9259..2517f756 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18352=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18352=01-02 Stats: 11 lines in 4 files changed: 5 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Thu, 21 Mar 2024 15:06:58 GMT, Jaikiran Pai wrote:
>> Bill Huang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Implemented review comments
>
> test/jdk/java/util/zip/ZipFile/ZeroDate.java line 95:
>
>> 93:
>> 94: // ensure that the archive is still readable, and the date is
>> 1979-11-30
>> 95: Path path = Utils.createTempFile("out", ".zip");
>
> So it looks like the test library has this utility method which allows to
> create temporary files within the jtreg scratch directory. Perhaps we should
> use it then. Having said that, is there a reason why one test method in this
> test now uses `Files.createTempFile(...)` and this other test method uses
> `Utils.createTempFile(...)`?
Yes, you are right. We don't need explicit deletion for these files by using
this util method, Utils.createTempFile.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534290137
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Thu, 21 Mar 2024 14:41:36 GMT, Jaikiran Pai wrote:
>> Bill Huang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Implemented review comments
>
> test/jdk/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java line 57:
>
>> 55:
>> 56: File flagsFile = File.createTempFile("CheckOriginFlags",
>> null);
>> 57: flagsFile.deleteOnExit();
>
> Hello Bill, jtreg uses a scratch directory when running tests. When a test is
> launched, the current working directory points to the scratch directory for
> the test that's currently executing. jtreg manages the lifecycle of scratch
> directories and even cleans them up (as necessary).
> Would it instead be better to just create the temporary file within the jtreg
> scratch directory (represented by the current working directory)? That way
> you could just do:
>
>
> File flagsFile = Files.createTempFile(Path.of("."), "CheckOriginFlags",
> null).toFile();
>
> and don't need any explicit deletions?
Hi Jaikiran, I think both solutions work for this bug. I personally prefer to
place the files in the scratch directory for the ease of debugging.
In addition, for this specific test, I am considering using
File.createTempFile("CheckOriginaFlags", null, Path.of(".").toFile) instead of
Files.createTempFile for consistency purposes, as Files.createTempFile may have
more restrictive access permissions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534283105
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implemented review comments
test/jdk/java/util/zip/ZipFile/ZeroDate.java line 95:
> 93:
> 94: // ensure that the archive is still readable, and the date is
> 1979-11-30
> 95: Path path = Utils.createTempFile("out", ".zip");
So it looks like the test library has this utility method which allows to
create temporary files within the jtreg scratch directory. Perhaps we should
use it then. Having said that, is there a reason why one test method in this
test now uses `Files.createTempFile(...)` and this other test method uses
`Utils.createTempFile(...)`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534105420
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implemented review comments
test/jdk/java/nio/channels/unixdomain/Bind.java line 201:
> 199: System.out.println("Null server address: " +
> server.getLocalAddress());
> 200: } finally {
> 201: Files.deleteIfExists(usa.getPath());
`usa` can be `null` here, if it never got assigned due to some exception in the
prior lines, which can lead to a `NullPointerException` here.
test/jdk/java/nio/channels/unixdomain/Bind.java line 341:
> 339: assertAddress(client.getRemoteAddress(), usa, "server");
> 340: } finally {
> 341: Files.deleteIfExists(usa.getPath());
Same applies here about potential NullPointerException and some other places in
other files as well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534092806
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534095046
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang wrote:
>> This task addresses an essential aspect of our testing infrastructure: the
>> proper handling and cleanup of temporary files and socket files created
>> during test execution. The motivation behind these changes is to prevent the
>> accumulation of unnecessary files in the default temporary directory, which
>> can affect the system's storage and potentially influence subsequent test
>> runs.
>>
>> Our review identified that several tests create temporary files or socket
>> files without ensuring their removal post-execution.
>> - Direct calls to java.io.File.createTempFile and
>> java.nio.file.Files.createTempFile without adequate cleanup.
>> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not
>> explicitly removing socket files post-use.
>
> Bill Huang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Implemented review comments
test/jdk/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java line 57:
> 55:
> 56: File flagsFile = File.createTempFile("CheckOriginFlags",
> null);
> 57: flagsFile.deleteOnExit();
Hello Bill, jtreg uses a scratch directory when running tests. When a test is
launched, the current working directory points to the scratch directory for the
test that's currently executing. jtreg manages the lifecycle of scratch
directories and even cleans them up (as necessary).
Would it instead be better to just create the temporary file within the jtreg
scratch directory (represented by the current working directory)? That way you
could just do:
File flagsFile = Files.createTempFile(Path.of("."), "CheckOriginFlags",
null).toFile();
and don't need any explicit deletions?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1534054951
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
On Tue, 19 Mar 2024 17:58:46 GMT, Bill Huang wrote: >> This task addresses an essential aspect of our testing infrastructure: the >> proper handling and cleanup of temporary files and socket files created >> during test execution. The motivation behind these changes is to prevent the >> accumulation of unnecessary files in the default temporary directory, which >> can affect the system's storage and potentially influence subsequent test >> runs. >> >> Our review identified that several tests create temporary files or socket >> files without ensuring their removal post-execution. >> - Direct calls to java.io.File.createTempFile and >> java.nio.file.Files.createTempFile without adequate cleanup. >> - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not >> explicitly removing socket files post-use. > > Bill Huang has updated the pull request incrementally with one additional > commit since the last revision: > > Implemented review comments Speaking for JFR, we should probably just create a normal file, possibly with a filename to indicate subtest and iteration. That said, test changes for JFR looks fine, and fixing the filename is outside the scope of this bug. - PR Comment: https://git.openjdk.org/jdk/pull/18352#issuecomment-2009218768
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests [v2]
> This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. Bill Huang has updated the pull request incrementally with one additional commit since the last revision: Implemented review comments - Changes: - all: https://git.openjdk.org/jdk/pull/18352/files - new: https://git.openjdk.org/jdk/pull/18352/files/8472c31f..620f9259 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18352=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18352=00-01 Stats: 136 lines in 5 files changed: 36 ins; 13 del; 87 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
On Mon, 18 Mar 2024 16:47:24 GMT, Bill Huang wrote: > This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. test/jdk/java/nio/channels/unixdomain/Bind.java line 191: > 189: server.bind(null); > 190: UnixDomainSocketAddress usa = > (UnixDomainSocketAddress)server.getLocalAddress(); > 191: usa.getPath().toFile().deleteOnExit(); The test already deletes the file, I think you just want a try-finally here, same comment on a few other tests. - PR Review Comment: https://git.openjdk.org/jdk/pull/18352#discussion_r1529408542
RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
This task addresses an essential aspect of our testing infrastructure: the proper handling and cleanup of temporary files and socket files created during test execution. The motivation behind these changes is to prevent the accumulation of unnecessary files in the default temporary directory, which can affect the system's storage and potentially influence subsequent test runs. Our review identified that several tests create temporary files or socket files without ensuring their removal post-execution. - Direct calls to java.io.File.createTempFile and java.nio.file.Files.createTempFile without adequate cleanup. - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not explicitly removing socket files post-use. - Commit messages: - Clean up temporary files after tests complete. Changes: https://git.openjdk.org/jdk/pull/18352/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18352=00 Issue: https://bugs.openjdk.org/browse/JDK-8327474 Stats: 42 lines in 11 files changed: 20 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352
