RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]

[Volker Simonis]

On Tue, 23 May 2023 05:34:15 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Volker Simonis has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>> 
>>  - Merge branch 'master' into JDK-8307990
>>  - Address comments from tstuefe and RogerRiggs
>>  - REALLY adding the test :)
>>  - Added test case
>>  - 8307990: jspawnhelper must close its writing side of a pipe before reading from it
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 28:
> 
>> 26:  * @test
>> 27:  * @bug 8307990
>> 28:  * @requires (os.family == "linux" & !vm.musl)
> 
> Muslc supports posix_spawn.
> 
> I tested your patch on Alpine, it works and the test works too. Please enable for musl.

Thanks, will do.

> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 92:
> 
>> 90:         }
>> 91:     }
>> 92: 
> 
> Small nits, mainly to make this test easier to understand to the casual reader:
> - consistent naming: we have "simulateCrashInChild" and "simulateCrashInParent", good, but then we have "childCode", which is actually the code executed in the parent, right?
> - simulateCrashInChild and simulateCrashInParent could be merged into one function that does:
>   - spawn parent with env var
>   - read output, scan for `"posix_spawn:XXX"`
>   - parent must have exit code != 0 and should not have timed out
>   - if XXX is not 0, check grandchild pid (must not be a live process)

This sounds reasonable. I've renamed `childCode()` to `parentCode()`, streamlined the wording for "parent" and "child" in the various log messages and exceptions and added a comment to the `main()` method which explains the working of the test and the meaning of "parent" and "child" in the output.

I've not merged `simulateCrashInChild()` and `simulateCrashInParent()` because there are some subtle differences between the two (and now with the improved "parent"/"child" naming even more :) and I don't think that a merged version will improve the readability.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1202266007
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1202365663