8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist

[weibxiao]

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change requires a CSR request to be approved

Issues

• JDK-8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist
• JDK-8294028: Produce warning when user specified java.io.tmpdir directory doesn't exist (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9989

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

Using diff file

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

[bridgekeeper[bot]]

:wave: Welcome back weibxiao! 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[bot]]

@weibxiao The following label will be automatically applied to this pull request:

• core-libs

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

[weibxiao]

CSR https://bugs.openjdk.org/browse/JDK-8293246

[mlbridge[bot]]

Webrevs

• 03: Full (2da71bdc)
• 02: Full - Incremental (abb19138)
• 01: Full - Incremental (cacf41d6)
• 00: Full (9f5a2983)

[weibxiao]

1. Remove the setting in java.security, not displaying the folder name of java.io.tmpdir
2. Add the change for NIO
3. Update the testing files.
4. In testing files, java.io.tmpdir is added as the option of command line

[AlanBateman]

Is the primary motive to catch scenarios where someone provides -Djava.io.tmpdir= on the command line? I asked this via a comment in the JBS issue last month and it would be useful to know if this is the scenario that you are interested in. If it is, then I think we should look at a warning at startup rather than delayed warning (or two warnings with the current patch).

[bridgekeeper[bot]]

@weibxiao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[openjdk[bot]]

@weibxiao this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout improve-directory-not-existing-error-message
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[weibxiao]

New approach,

1. Print warning message inside StaticProperty.java when JVM start
2. Remove the print-out inside TempFileHelper.java and File.java
3. Keep one test file as TempDirectoryNotExisting
4. The order of the code was changed in System.java since StaticProperty.java needs "out print-stream" to be initialized before using it to print a warning message.

[RogerRiggs]

The check for the tmp directory should be delayed until it is the property is used. If temp files are not used, it only increases startup time without any benefit (usually the directory exists).

Putting the check and printing a warning in the static initialization of Files.TempDirectory will delay its evaluation until it is needed and the initialization order in System will not need to be changed. Changing the initialization order is always has some risks.

[AlanBateman]

The changes in the latest patch don't make sense, maybe there is a merge error? From what I can see, I don't think you should be changing initPhase1 or changing StaticProperty. to print the warning. A possible starting point for this PR would be to focus on the case that java.io.tmpdir is configured on the command line. If it is, then it would be fair game to eagerly check that it is a directory. One candidate place to emit the warning is initPhase3 where you'll see several other warnings. The other case is the user hasn't configured java.io.tmpdir, in which case it will default to /var/tmp (or whatever) is not a directory. Do you really want the JDK to be checking this every time? What happened to the exploration into extending the security property jdk.includeInExceptions?

[coffeys]

Seems like there's a difference of opinion on when such a configuration issue should be reported. Reporting at start up will cost cycles (esp since there's an IO stat call) Maybe we can revert the patch to the original style which is to report the issue at time of tmpFile creation failure.

The original intent of the bug is to improve the exception message reported to end user for the extremely rare case of when java.io.tmpdir contains a bad value. The current exception is something like:

java.io.IOException: No such file or directory
        at java.io.UnixFileSystem.createFileExclusively(Native Method)

The exception message needs to suggest that the java.io.tmpdir property is a bad one. (no need to even print the directory value IMO)

[AlanBateman]

Seems like there's a difference of opinion on when such a configuration issue should be reported. Reporting at start up will cost cycles (esp since there's an IO stat call) Maybe we can revert the patch to the original style which is to report the issue at time of tmpFile creation failure.

The original intent of the bug is to improve the exception message reported to end user for the extremely rare case of when java.io.tmpdir contains a bad value.

I don't think we have any data on how common it change java.io.tmpdir to use a different location but if it rare then it should be okay to check the value eagerly. I think the issue we've had with all iterations of this PR to date is that it doesn't distinguish the case where the configuration is overridden, e.g. the current patch checks the location even when java.io.tmpdir is not changed; I guess this is what Roger is pointing out.

Yes, improving the exception and maybe use of jdk.includeInExceptions was listed as an option in JBS.

[weibxiao]

StaticProperty is using System.err to print out a warning message. PrintStream err needs to be initialized before being used by StaticPropery.

Going to revise the change according to the comments

[bridgekeeper[bot]]

@weibxiao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[weibxiao]

Close this one, Using https://github.com/openjdk/jdk/pull/11174 instead