Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

weibxiao
Copy link
Contributor

@weibxiao weibxiao commented Aug 23, 2022


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
  • 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
Copy link

bridgekeeper bot commented Aug 23, 2022

👋 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
Copy link

openjdk bot commented Aug 23, 2022

@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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 23, 2022
@weibxiao weibxiao changed the title Improve directory not existing error message 8290313: Improve directory not existing error message Aug 23, 2022
@weibxiao weibxiao changed the title 8290313: Improve directory not existing error message 8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist Aug 23, 2022
@@ -2176,6 +2176,12 @@ public static File createTempFile(String prefix, String suffix,
}
} while (fs.hasBooleanAttributes(f, FileSystem.BA_EXISTS));


if(!tmpdir.exists() || !tmpdir.canWrite()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check that tmpdir.isDirectory() that will do both existence & directory check? That would catch if java.io.t odor was mistakenly set to an existing (writable) file.

Suggested change
if(!tmpdir.exists() || !tmpdir.canWrite()) {
if(!tmpdir.isDirectory() || !tmpdir.canWrite()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a new approach, please check the new commits. Thanks.

@@ -2176,6 +2176,12 @@ public static File createTempFile(String prefix, String suffix,
}
} while (fs.hasBooleanAttributes(f, FileSystem.BA_EXISTS));


if(!tmpdir.exists() || !tmpdir.canWrite()) {
throw new IOException("The specified directory does no exist or is not accessible! " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling nit:

Suggested change
throw new IOException("The specified directory does no exist or is not accessible! " +
throw new IOException("The specified directory does not exist or is not accessible! " +

@weibxiao
Copy link
Contributor Author

weibxiao commented Sep 7, 2022

@weibxiao weibxiao marked this pull request as ready for review September 7, 2022 15:16
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 7, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 7, 2022

Webrevs

static {
if (tmpdir.isInvalid() || !fs.hasBooleanAttributes(tmpdir, FileSystem.BA_DIRECTORY)) {
String msg = "WARNING: java.io.tmpdir location does not exist";
if (SecurityProperties.includedInExceptions("fileInfo")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security property controls is for exception messages, not warnings to System.err.

@@ -1986,6 +1987,18 @@ private TempDirectory() { }
// temporary directory location
private static final File tmpdir = new File(
GetPropertyAction.privilegedGetProperty("java.io.tmpdir"));

// print out the error message for java.io.tmpdir setting
static {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only run when java.io.File.createTempFile is used. So it work won't work java.nio.files.Files.createTempXXX. Have you looked into testing the value of java.io.tmpdir at startup when it is specified on the command line?

@weibxiao
Copy link
Contributor Author

weibxiao commented Sep 8, 2022

  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
Copy link
Contributor

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).

class TempFileHelper {
private TempFileHelper() { }

// temporary directory location
private static final Path tmpdir = Path.of(StaticProperty.javaIoTmpDir());

// print out the error message for java.io.tmpdir setting
static {
if (!Files.isDirectory(tmpdir)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This this work with a security manager set?


import java.io.File;

public class TempDirectoryNotExistingErrorMessage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests look the same and maybe we can get away with one in java/io/File. Renaming it something like The TempDirectoryDoesNotExist would make it a bit more consistent with the tests in the Files test directory.

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Sep 21, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 7, 2022

@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
Copy link

openjdk bot commented Oct 14, 2022

@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

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 14, 2022
@openjdk openjdk bot removed the rfr Pull request is ready for review label Oct 14, 2022
@openjdk openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Oct 14, 2022
@weibxiao
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

coffeys commented Oct 15, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Comment on lines +72 to +76

if (!(new File(JAVA_IO_TMPDIR)).isDirectory()) {
System.err.println("WARNING: java.io.tmpdir location does not exist");
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.out/err relies on encoding properties are initialized, so I don't think this would work.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 14, 2022

@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
Copy link
Contributor Author

Close this one, Using #11174 instead

@weibxiao weibxiao closed this Nov 17, 2022
@weibxiao weibxiao deleted the improve-directory-not-existing-error-message branch March 6, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review
6 participants