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 #11174

Conversation

weibxiao
Copy link
Contributor

@weibxiao weibxiao commented Nov 15, 2022

print warning message for java.io.tmpdir when it is set through the command line and the value is not good for creating file folder.


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-8290313: Produce warning when user specified java.io.tmpdir directory doesn't exist

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11174

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 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 Nov 15, 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 core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration labels Nov 15, 2022
@weibxiao weibxiao marked this pull request as ready for review November 15, 2022 20:20
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 15, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 15, 2022

Webrevs

@@ -2244,6 +2244,9 @@ private static void initPhase3() {
// SecurityManager
Unsafe.getUnsafe().ensureClassInitialized(StringConcatFactory.class);

// print out warning message if java.io.tmpdir is set through command line with non-existing folder
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably okay to have the warning emitted in checkIoTmpDir, just a bit out of sight and inconsistent with the other warnings printed in this class.

I think the comment could be improved by bit with "Emit a warning if java.io.tmpdir is set via the command line to a directory that doesn't exist".

@naotoj
Copy link
Member

naotoj commented Nov 15, 2022

I just wonder if this change warrants a CSR. If it is merely printing out a warning, it may not require it IMO. Otherwise, if some real behavioral change exists, or clarifying the behavior, I think the java.io.tmpdir in javadoc should mention it.

@weibxiao
Copy link
Contributor Author

  1. Removed the link for CSR.
  2. Add a new method in SystemProps.java to check if the warning message is required
  3. Print the warning message directly in System::initPhase3

@@ -54,6 +67,8 @@ public static Map<String, String> initProperties() {
Raw raw = new Raw();
HashMap<String, String> props = raw.cmdProperties();

customTmpdir = props.get("java.io.tmpdir");
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the assignment to line 98:

customTmpdir = putIfAbsent(props, "java.io.tmpdir", raw.propDefault(Raw._java_io_tmpdir_NDX));

It will return null if the property is not already set and save a little bit.
If it is set, it will return the custom directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I retract this suggestion, the putIfAbsent method does not return a value.
Moving the assignment to before line 113, would keep the references to java.io.tmpdir together.

*
* @return a boolean value
*/
public static boolean checkIfWarningRequired() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename the method to checkIoTmpdir; it will be easier to see the purpose in the call in System.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I might suggest isBadIoTmpdir for method name

private static String customTmpdir;

/**
* check if warning for custom java.io.tmpdir is required
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitialize and add a period at the end.

@@ -2244,6 +2244,11 @@ private static void initPhase3() {
// SecurityManager
Unsafe.getUnsafe().ensureClassInitialized(StringConcatFactory.class);

// Emit a warning if java.io.tmpdir is set via the command line to a directory that doesn't exist
if (SystemProps.checkIfWarningRequired()) {
System.err.println("WARNING: java.io.tmpdir location does not exist");
Copy link
Contributor

Choose a reason for hiding this comment

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

Change "location" to "directory", reflecting the exact check being done.

public static void main(String... args) throws Exception {

String userDir = System.getProperty("user.home");
String timeStamp = System.currentTimeMillis() + "";
Copy link
Contributor

Choose a reason for hiding this comment

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

A human readable string might be useful. For example, "2022-11-16T15:10:50.622334Z"
java.time.Instant.now().toString()

Copy link
Contributor

Choose a reason for hiding this comment

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

root cause for JDK-8297528

Please revert the change Weibing and be sure to remove the test from ProblemList when doing so (next week)

Please also ensure that you run tests against mach5 before integrating any change in future,

String tempDir = Path.of(userDir,"non-existing-", timeStamp).toString();

if (args.length != 0) {
if (args[0].equals("io")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code to cover the cases could be a bit cleaner and without duplication as:

    for (String arg : args) {
        if (arg.equals("io")) {
            ...
        } else if (arg.equals("nio")) { 
            ...
        } else { 
           throw Exception("unknown case: " + arg);
        }
    }

.asLines().stream()
.filter(line -> line.equalsIgnoreCase(ioWarningMsg))
.collect(Collectors.toList());
if (list.size() != 1) throw new Exception("counter of messages is not one, but " + list.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's an error, print the list of messages; it can be very informative to see the unexpected messages, including the case where the exitValue is wrong.

@jaikiran
Copy link
Member

Hello @weibxiao, I had a look at the linked JBS issue https://bugs.openjdk.org/browse/JDK-8290313. It appears to me that the original motivation of this change is to improve the situation where a call to File.createTempFile(...) can fail with the following excepiton:

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

if either the java.io.tmpdir doesn't exist or the user passed value for the directory parameter to File.createTempDirectory(prefix, suffix, directory) doesn't exist.

I feel that the change being proposed here to print a warning if the user specified java.io.tmpdir won't improve this situation because:

  • This warning will get printed potentially (far) away and early in the logs as compared to where this exception might get logged.
  • The user may not even have access to the log file, and it just might be the case that the application code caught this exception and printed its stacktrace
  • This will continue to fail with the above exception (and without the proposed warning) if the user passed directory to the File.createTempDirectory(prefix, suffix, directory) is non-existent. For example (/tmp/blah directory doesn't exist in the example below):
jshell> File.createTempFile("foo", null, new File("/tmp/blah"));
|  Exception java.io.IOException: No such file or directory
|        at UnixFileSystem.createFileExclusively0 (Native Method)
|        at UnixFileSystem.createFileExclusively (UnixFileSystem.java:356)
|        at File.createTempFile (File.java:2179)
|        at (#2:1)

I think if we want to improve this error message then maybe attempt the solution proposed by Alan in the JBS issue:

Extend jdk.includeInExceptions to include "file", meaning opt-in in secure deployments to include the tmp directory in the exception.

or maybe change the code in File.createTempFile(...) to something like:

diff --git a/src/java.base/share/classes/java/io/File.java b/src/java.base/share/classes/java/io/File.java
index 8f1e88219fe..4eb9db40e9d 100644
--- a/src/java.base/share/classes/java/io/File.java
+++ b/src/java.base/share/classes/java/io/File.java
@@ -2157,7 +2157,9 @@ public class File
 
         File tmpdir = (directory != null) ? directory
                                           : TempDirectory.location();
-
+        if (!tmpdir.isDirectory()) {
+            throw new IOException("Parent directory doesn't exist, cannot create a temporary file");
+        }
         @SuppressWarnings("removal")
         SecurityManager sm = System.getSecurityManager();
         File f;

I haven't given much thought to the diff I proposed here, so this obviously will need more experimentation and inputs from others whether it's worth pursuing. Additionally, if we do decide to improve the exception being thrown, perhaps we should also review the code in java.nio.file.Files.createTemp.... methods and the exceptions they throw.

@AlanBateman
Copy link
Contributor

I think if we want to improve this error message then maybe attempt the solution proposed by Alan in the JBS issue:

Extend jdk.includeInExceptions to include "file", meaning opt-in in secure deployments to include the tmp directory in the exception.

Yeah, I listed three suggestions in the JBS issue. This PR is item 2, which is to emit a warning a startup when the running with -Djava.io.tmpdir set to a bad directory. That is useful on its own and should help in some scenarios. Item 1 is the opt-in to improve the exception and I think that can be done too with another PR.

@mlbridge
Copy link

mlbridge bot commented Nov 17, 2022

Mailing list message from Jaikiran Pai on core-libs-dev:

On 17/11/22 1:36 pm, Alan Bateman wrote:

On Thu, 17 Nov 2022 07:41:09 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

I think if we want to improve this error message then maybe attempt the solution proposed by Alan in the JBS issue:

Extend jdk.includeInExceptions to include "file", meaning opt-in in secure deployments to include the tmp directory in the exception.
Yeah, I listed three suggestions in the JBS issue. This PR is item 2, which is to emit a warning a startup when the running with -Djava.io.tmpdir set to a bad directory. That is useful on its own and should help in some scenarios.

Thank you Alan for that detail. I didn't realize this was an independent
change being pursued.

-Jaikiran

@AlanBateman
Copy link
Contributor

The src changes in update ede6c42 look okay. The test name is a bit strange, maybe a name like TempDirDoesNotExist.java would work better.

@@ -54,6 +67,8 @@ public static Map<String, String> initProperties() {
Raw raw = new Raw();
HashMap<String, String> props = raw.cmdProperties();

customTmpdir = props.get("java.io.tmpdir");
Copy link
Contributor

Choose a reason for hiding this comment

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

I retract this suggestion, the putIfAbsent method does not return a value.
Moving the assignment to before line 113, would keep the references to java.io.tmpdir together.

Comment on lines 63 to 73
} else if (arg.equals("io-nio")) {
try {
File.createTempFile("prefix", ".suffix");
} catch (Exception e) {
e.printStackTrace();
}
try {
Files.createTempFile("prefix", ".suffix");
} catch (Exception e) {
e.printStackTrace();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This repetition of the code is not necessary if invoked with the two arguments "io" and "nio".

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

LGTM

throw new Exception("counter of messages is not one, but " + list.size()
+ "\n" + originalOutput.asLines().toString());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: need a newline at the end

@@ -2244,6 +2244,11 @@ private static void initPhase3() {
// SecurityManager
Unsafe.getUnsafe().ensureClassInitialized(StringConcatFactory.class);

// Emit a warning if java.io.tmpdir is set via the command line to a directory that doesn't exist
Copy link
Contributor

Choose a reason for hiding this comment

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

could you split this comment across 2 lines ? - Keeps length similar to comment just above this code.

* @summary Produce warning when user specified java.io.tmpdir directory doesn't exist
*/

import jdk.test.lib.Platform;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

@weibxiao
Copy link
Contributor Author

/csr unneeded

@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@weibxiao only Reviewers can determine that a CSR is not needed.

@coffeys
Copy link
Contributor

coffeys commented Nov 23, 2022

/csr unneeded

A release note should be sufficient for this change

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 23, 2022
@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@coffeys determined that a CSR request is not needed for this pull request.

@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@weibxiao 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:

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

Reviewed-by: rriggs, naoto, coffeys

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 344 new commits pushed to the master branch:

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @naotoj, @coffeys) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 23, 2022
@weibxiao
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 23, 2022
@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@weibxiao
Your change (at version ba12fc0) is now ready to be sponsored by a Committer.

@coffeys
Copy link
Contributor

coffeys commented Nov 23, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 23, 2022

Going to push as commit 8df3bc4.
Since your change was applied there have been 344 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 23, 2022
@openjdk openjdk bot closed this Nov 23, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 23, 2022
@openjdk
Copy link

openjdk bot commented Nov 23, 2022

@coffeys @weibxiao Pushed as commit 8df3bc4.

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

@dcubed-ojdk
Copy link
Member

The new test, java/io/File/TempDirDoesNotExist.java, is failing on windows-x64.
See JDK-8297528 new java/io/File/TempDirDoesNotExist.java test failing on windows-x64

@mlbridge
Copy link

mlbridge bot commented Nov 24, 2022

Mailing list message from Roger Riggs on core-libs-dev:

Please review a PR to put the test on the ProblemList until the test is
fixed.

https://github.com//pull/11334

Thanks, Roger

On 11/23/22 3:27 PM, Daniel D. Daugherty wrote:

@weibxiao weibxiao deleted the improve-temp-dir-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 integrated Pull request has been integrated
7 participants