On Tue, 6 May 2025 11:23:32 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Aleksey Shipilev has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains six additional
>> commits since the last revision:
>>
>> - Add bug ID
>> - Merge branch 'master' into JDK-8352533-jspawnhelper-ioexceptions
>> - Pull message into a field
>> - Test cases
>> - Merge branch 'master' into JDK-8352533-jspawnhelper-ioexceptions
>> - Initial fix
>>
>> Good spawnhelper failure message
>>
>> Trying tests
>>
>> Print helper message on all IOExceptions
>>
>> Final
>
> src/java.base/unix/native/libjava/ProcessImpl_md.c line 346:
>
>> 344: if (ret != EINVAL)
>> 345: detail = tmpbuf;
>> 346: }
>
> Pre-existing, possibly as follow-up:
>
> I wonder whether we can do more. I don't like how we either only print out
> the errno string, or only print out the "Default" detail (I don't like the
> "default" prefix since it's really useful context information about this
> errno).
>
> For a problem on this side, we mostly pass in errno, then swallow the
> "defaultdetail". E.g. for posix_spawn failure, we'd print e.g. "12:Out of
> memory\nPossible reasons: ... " (or whatever localized string strerror
> returns).
>
> For errors that happen inside the jspawnhelper (see line 790ff where we pass
> 0 for the errno), we print out "0:Bad code from spawn helper\nPossible
> reasons: ... ", so we swallow the errno we got from the jspawnhelper.
>
> Could we print out instead the default, then the errno string, then the
> Possible reasons text? E.g.
> "posix_spawn failed (12:Out of memory)\nPossible reasons: ..."
> or
> "Bad code from spawn helper (12:Out of memory)\nPossible reasons: ..."
>
> There is an issue of errnum values from the far side possibly intersecting
> with valid errno. Could be solved simply by assigning negative values to
> ERR_MALLOC, ERR_PIPE and ERR_ARGS. I am not aware of any errno values being
> negative. The errno printing would need to recognise these three values in
> addition to errno values. We also could remove the +3 workaround in the test.
>
> The only argument I see against is some vague form of backward compatibility,
> in case people get confused about the new information, or that existing tools
> parse this output.
Yeah, I dislike `defaultDetail` naming and the fact we swallow it. I see good
reason to print it unconditionally. I think we can do this with just a little
code massaging. See new commit. It prints the message like:
Recursively executing 'JspawnhelperProtocol simulateCrashInChild4'
posix_spawn:0
java.io.IOException: Cannot run program "pwd": Failed to exec spawn helper:
pid: 1405770, exit code: 4, error: 0 (none)
Possible reasons:
- Spawn helper ran into JDK version mismatch
- Spawn helper ran into unexpected internal error
- Spawn helper was terminated by another process
Possible solutions:
- Restart JVM, especially after in-place JDK updates
- Check system logs for JDK-related errors
- Re-install JDK to fix permission/versioning problems
- Switch to legacy launch mechanism with
-Djdk.lang.Process.launchMechanism=VFORK
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1110)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1044)
at java.base/java.lang.Runtime.exec(Runtime.java:605)
at java.base/java.lang.Runtime.exec(Runtime.java:470)
at JspawnhelperProtocol.parentCode(JspawnhelperProtocol.java:58)
at JspawnhelperProtocol.main(JspawnhelperProtocol.java:232)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24149#discussion_r2086480179
