On Tue, 23 May 2023 05:34:15 GMT, Thomas Stuefe <stu...@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
