JDK-8327474 Review use of java.io.tmpdir in jdk tests #18352

bwhuang-us · 2024-03-18T16:47:24Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327474: Review use of java.io.tmpdir in jdk tests (Bug - P4)

Reviewers

Michael McMahon (@Michael-Mc-Mahon - Reviewer) ⚠️ Review applies to 2517f756
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352
$ git checkout pull/18352

Update a local copy of the PR:
$ git checkout pull/18352
$ git pull https://git.openjdk.org/jdk.git pull/18352/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18352

View PR using the GUI difftool:
$ git pr show -t 18352

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18352.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-18T16:47:56Z

👋 Welcome back bhuang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-18T16:49:02Z

@bwhuang-us This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327474: Review use of java.io.tmpdir in jdk tests

Reviewed-by: michaelm, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 254 new commits pushed to the master branch:

866e7b6: 8329174: update CodeBuffer layout in comment after constants section moved
f88f31d: 8328137: PreserveAllAnnotations can cause failure of class retransformation
021ed6a: 8328648: Remove applet usage from JFileChooser tests bug4150029
3057dde: 8329421: Native methods can not be selectively printed
db15914: 8328753: Open source few Undecorated Frame tests
925d829: 8329013: StackOverflowError when starting Apache Tomcat with signed jar
dd5d7d0: 8327002: (fs) java/nio/file/Files/CopyMoveVariations.java should be able to test across file systems
6ae1cf1: 8329368: Generational ZGC: Remove the unnecessary friend classes in ZAllocator
7eb78e3: 8320676: Manual printer tests have no Pass/Fail buttons, instructions close set 1
5ac067f: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary
... and 244 more: https://git.openjdk.org/jdk/compare/e6a8fdd82c2b97f7ae74dfe8fbd3402718c9161c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-18T16:49:25Z

@bwhuang-us The following labels will be automatically applied to this pull request:

core-libs
hotspot-jfr
jmx
nio
security
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-18T16:52:46Z

Webrevs

test/jdk/java/nio/channels/unixdomain/Bind.java

egahlin · 2024-03-20T10:25:05Z

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.

test/jdk/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java

test/jdk/java/nio/channels/unixdomain/Bind.java

test/jdk/java/util/zip/ZipFile/ZeroDate.java

seanjmullan · 2024-03-21T17:43:55Z

The bug should have a noreg-self label.

test/jdk/java/nio/channels/unixdomain/Bind.java

Michael-Mc-Mahon

unixdomain NIO test changes look fine to me

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

jaikiran · 2024-04-02T14:46:20Z

test/jdk/java/nio/channels/unixdomain/NonBlockingAccept.java

            SocketChannel socketChannel = serverSocketChannel.accept();
            System.out.println("The socketChannel is : expected Null " + socketChannel);
            if (socketChannel != null)
                throw new RuntimeException("expected null");
            // or exception could be thrown otherwise
+        } finally {
+            Files.deleteIfExists(addr.getPath());


Hello Bill, addr can be null here, so this has a potential of a NullPointerException.

jaikiran · 2024-04-02T14:49:58Z

test/jdk/sun/security/pkcs12/P12SecretKey.java

@@ -66,6 +66,7 @@ private void run(String keystoreType, String algName, int keySize) throws Except
        ks.setEntry(ALIAS, ske, kspp);

        File ksFile = File.createTempFile("test", ".test");
+        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?

jaikiran · 2024-04-02T14:53:02Z

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.

jaikiran

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.

bwhuang-us · 2024-04-03T17:02:57Z

/integrate

openjdk · 2024-04-03T17:04:10Z

Going to push as commit 375bfac.
Since your change was applied there have been 266 commits pushed to the master branch:

233619b: 8329557: Fix statement around MathContext.DECIMAL128 rounding
023f7f1: 8320799: Bump minimum boot jdk to JDK 22
8dc43aa: 8325217: MethodSymbol.getModifiers() returns SEALED for restricted methods
1c69193: 8328383: Method is not used: com.sun.tools.javac.comp.Attr::thisSym
ee09801: 8328352: Serial: Inline SerialBlockOffsetSharedArray
bea493b: 8236736: Change notproduct JVM flags to develop flags
80c54b4: 8328932: Parallel: Proper partial object setup in fill_dense_prefix_end
d954f3b: 8329493: Parallel: Remove unused ParallelArguments::heap_max_size_bytes
bdd9438: 8328647: TestGarbageCollectorMXBean.java fails with C1-only and -Xcomp
e3e6c2a: 8328278: Do not print the tenuring threshold in AgeTable::print_on
... and 256 more: https://git.openjdk.org/jdk/compare/e6a8fdd82c2b97f7ae74dfe8fbd3402718c9161c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-03T17:04:16Z

@bwhuang-us Pushed as commit 375bfac.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Clean up temporary files after tests complete.

8472c31

openjdk bot added the rfr Pull request is ready for review label Mar 18, 2024

openjdk bot added security security-dev@openjdk.org serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org labels Mar 18, 2024

AlanBateman reviewed Mar 18, 2024

View reviewed changes

test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved

Implemented review comments

620f925

jaikiran reviewed Mar 21, 2024

View reviewed changes

test/jdk/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java Outdated Show resolved Hide resolved

jaikiran reviewed Mar 21, 2024

View reviewed changes

test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved

jaikiran reviewed Mar 21, 2024

View reviewed changes

test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved

jaikiran reviewed Mar 21, 2024

View reviewed changes

test/jdk/java/util/zip/ZipFile/ZeroDate.java Show resolved Hide resolved

Fixed potential NPE issues.

2517f75

turbanoff reviewed Mar 26, 2024

View reviewed changes

test/jdk/java/nio/channels/unixdomain/Bind.java Outdated Show resolved Hide resolved

Michael-Mc-Mahon approved these changes Mar 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 26, 2024

Update test/jdk/java/nio/channels/unixdomain/Bind.java

0f4130a

Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

jaikiran reviewed Apr 2, 2024

View reviewed changes

Implemented review comments.

8124460

jaikiran approved these changes Apr 3, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 3, 2024

openjdk bot closed this Apr 3, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8327474 Review use of java.io.tmpdir in jdk tests #18352

JDK-8327474 Review use of java.io.tmpdir in jdk tests #18352

bwhuang-us commented Mar 18, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 18, 2024

openjdk bot commented Mar 18, 2024 •

edited

Loading

openjdk bot commented Mar 18, 2024

mlbridge bot commented Mar 18, 2024 •

edited

Loading

egahlin commented Mar 20, 2024

seanjmullan commented Mar 21, 2024

Michael-Mc-Mahon left a comment

jaikiran Apr 2, 2024

jaikiran Apr 2, 2024

jaikiran commented Apr 2, 2024

jaikiran left a comment

bwhuang-us commented Apr 3, 2024

openjdk bot commented Apr 3, 2024

openjdk bot commented Apr 3, 2024

JDK-8327474 Review use of java.io.tmpdir in jdk tests #18352

JDK-8327474 Review use of java.io.tmpdir in jdk tests #18352

Conversation

bwhuang-us commented Mar 18, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 18, 2024

openjdk bot commented Mar 18, 2024 • edited Loading

openjdk bot commented Mar 18, 2024

mlbridge bot commented Mar 18, 2024 • edited Loading

Webrevs

egahlin commented Mar 20, 2024

seanjmullan commented Mar 21, 2024

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

jaikiran Apr 2, 2024

Choose a reason for hiding this comment

jaikiran Apr 2, 2024

Choose a reason for hiding this comment

jaikiran commented Apr 2, 2024

jaikiran left a comment

Choose a reason for hiding this comment

bwhuang-us commented Apr 3, 2024

openjdk bot commented Apr 3, 2024

openjdk bot commented Apr 3, 2024

bwhuang-us commented Mar 18, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 18, 2024 •

edited

Loading

mlbridge bot commented Mar 18, 2024 •

edited

Loading