8379967: (process) Improve ProcessBuilder error reporting#30232
8379967: (process) Improve ProcessBuilder error reporting#30232tstuefe wants to merge 9 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
|
Ping @RogerRiggs |
|
@tstuefe This change is no longer ready for integration - check the PR body for details. |
1fd31f8 to
8f950b4
Compare
8f950b4 to
5afcf2b
Compare
|
@tstuefe |
Webrevs
|
|
/label remove hotspot |
|
@tstuefe |
|
@tstuefe |
|
@tstuefe |
|
@tstuefe |
|
@tstuefe |
|
@tstuefe |
RogerRiggs
left a comment
There was a problem hiding this comment.
Looks good, A bit more detailed than I was thinking but is should be worth it when things take an unexpected turn.
| * @requires vm.flagless | ||
| * @library /test/lib | ||
| * @run main/othervm -Xmx64m -Djdk.lang.Process.launchMechanism=FORK InvalidWorkDir | ||
| * @summary Check that passing an invalid work dir yields a corresponding IOE text. |
There was a problem hiding this comment.
The tag order is usually @test, @bug, @summary, @library, @requires, @run.
| public class InvalidWorkDir { | ||
|
|
||
| public static void main(String[] args) { | ||
| ProcessBuilder bld = new ProcessBuilder("ls").directory(new File("/doesnotexist")); |
There was a problem hiding this comment.
Using the / root directory seems unusual, the current directory (for tests) is safe and the directory is cleared before running.
I am just sick of outguessing the jspawnhelper logic :-) |
RogerRiggs
left a comment
There was a problem hiding this comment.
Add bugid's and you're good to go.
Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>
Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>
Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>
Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>
|
Thank you, Roger! Let's see if Skara lets me integrate without re-approval. /integrate |
|
@RogerRiggs Need a re-review :-( |
|
/integrate |
|
@tstuefe This pull request has not yet been marked as ready for integration. |
While working on a patch for https://bugs.openjdk.org/browse/JDK-8377907, we saw that error analysis in this area can take an unreasonable amount of time, especially for cases that are not directly reproducible on a developer machine.
This is because if an error occurs in the child process between the fork and exec stages (FORK mode) or between the first and second exec stages (POSIX_SPAWN mode), the ways in which we can communicate the errors are quite limited: standard IO may not yet work, and there is no logging. All we have is a single 32-bit value that we send back to the parent.
Today, this 32-bit value contains errno or a handful of our own error codes (which even overlap with errno values).
Following an idea by @RogerRiggs, this patch enhances the returned code by encoding more information:
The system is clean and easily expandable with more detailed error information, should we need it. We can also bump the error code to 64-bit to get more encoding space, but I left it at 32-bit for now.
Error handling:
When an error occurs, we now attempt to send the error code back to the parent as before, but if that fails (e.g., because the fail pipe was not established), we also print the error code and exit the child process with an exit code corresponding to the step number.
Where we print the error code, we use the form
(<step>-<hint>-<errno>). For example, "(2-0-0)" is a jspawnhelper version mismatch. "(3-17-9)" means that in step 3 (early jspawnhelper setup), we found that file descriptor 17 was invalid (9=EBADF). We only do this for internal errors that are meant to be read by us, not by the end user.As a by-product of this patch, we now get higher error granularity for some user-facing errors. E.g., if the caller handed in an invalid working directory, the IOE exception message now reads "Invalid working directory" instead of the generic "Exec failed".
Implementation notes:
ESTEP_CHILD_ALIVE; but it works the same.boolin functions I changed, since I think it reads cleaner. Apart from that, I kept changes as small as possible.Testing:
Note: I intend to push this fix first, and then rebase #29939 to use it. That will make it easier for us to hunt down the remaining issues in #29939.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30232/head:pull/30232$ git checkout pull/30232Update a local copy of the PR:
$ git checkout pull/30232$ git pull https://git.openjdk.org/jdk.git pull/30232/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30232View PR using the GUI difftool:
$ git pr show -t 30232Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30232.diff
Using Webrev
Link to Webrev Comment