Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v6]
> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified > unexpected messages from a child Java VM > as the cause of the test failure. Attempts to control the output of the > child VM have failed, the VM is unrepentant . > > There is no functionality in the child except to wait long enough for the > test to finish and the child is destroyed. > The fix is to switch from using a Java child to using a native child; a new > executable `sleepmillis`. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: The switch from a Java child to /bin/sleep caused another test to fail on Linux. The cleanup for a test used /usr/bin/pkill "sleep 60". A race between that cleanup and subsequent tests that used sleep 60 or 600 could kill the sleep before the test of waitFor completed. Changing the test using pkill to use 59 seconds makes the test cleanup selective to the sleep spawned for that test. The test that was failing (lines 2626-2624) passes consistently. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5239/files - new: https://git.openjdk.java.net/jdk/pull/5239/files/43a54802..afa932d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5239&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5239&range=04-05 Stats: 25 lines in 1 file changed: 7 ins; 10 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5239.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5239/head:pull/5239 PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v6]
On Mon, 20 Sep 2021 13:01:29 GMT, Roger Riggs wrote:
>> The intermittent test in java/lang/ProcessBuilder/Basic.java has identified
>> unexpected messages from a child Java VM
>> as the cause of the test failure. Attempts to control the output of the
>> child VM have failed, the VM is unrepentant .
>>
>> There is no functionality in the child except to wait long enough for the
>> test to finish and the child is destroyed.
>> The fix is to switch from using a Java child to using a native child; a new
>> executable `sleepmillis`.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> The switch from a Java child to /bin/sleep caused another test
> to fail on Linux. The cleanup for a test used /usr/bin/pkill "sleep 60".
> A race between that cleanup and subsequent tests that used sleep 60 or 600
> could kill the sleep before the test of waitFor completed.
> Changing the test using pkill to use 59 seconds makes the test cleanup
> selective to the sleep spawned for that test.
> The test that was failing (lines 2626-2624) passes consistently.
Hi Roger,
One suggestion based on your latest change, but that can be handled later.
Otherwise this seems fine.
Thanks,
David
test/jdk/java/lang/ProcessBuilder/Basic.java line 2217:
> 2215: // A unique (59s) time is needed to avoid killing other
> sleep processes.
> 2216: final String[] cmd = { "/bin/bash", "-c", "(/bin/sleep
> 59)" };
> 2217: final String[] cmdkill = { "/bin/bash", "-c",
> "(/usr/bin/pkill -f \"sleep 59\")" };
Maybe future RFE but why do we even need pkill here when we can get the PID of
the sleep process we create and kill only that process?
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v6]
On Tue, 21 Sep 2021 04:54:24 GMT, David Holmes wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> The switch from a Java child to /bin/sleep caused another test
>> to fail on Linux. The cleanup for a test used /usr/bin/pkill "sleep 60".
>> A race between that cleanup and subsequent tests that used sleep 60 or 600
>> could kill the sleep before the test of waitFor completed.
>> Changing the test using pkill to use 59 seconds makes the test cleanup
>> selective to the sleep spawned for that test.
>> The test that was failing (lines 2626-2624) passes consistently.
>
> test/jdk/java/lang/ProcessBuilder/Basic.java line 2217:
>
>> 2215: // A unique (59s) time is needed to avoid killing
>> other sleep processes.
>> 2216: final String[] cmd = { "/bin/bash", "-c", "(/bin/sleep
>> 59)" };
>> 2217: final String[] cmdkill = { "/bin/bash", "-c",
>> "(/usr/bin/pkill -f \"sleep 59\")" };
>
> Maybe future RFE but why do we even need pkill here when we can get the PID
> of the sleep process we create and kill only that process?
I thought of that too, but notice the parens "()" around that /bin/sleep; that
creates and extra level of forked processes and its harder to get that pid.
There probably is a way to traverse the hierarchy but I'll keep it as is for
now.
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v6]
On Tue, 21 Sep 2021 13:10:57 GMT, Roger Riggs wrote:
>> test/jdk/java/lang/ProcessBuilder/Basic.java line 2217:
>>
>>> 2215: // A unique (59s) time is needed to avoid killing
>>> other sleep processes.
>>> 2216: final String[] cmd = { "/bin/bash", "-c",
>>> "(/bin/sleep 59)" };
>>> 2217: final String[] cmdkill = { "/bin/bash", "-c",
>>> "(/usr/bin/pkill -f \"sleep 59\")" };
>>
>> Maybe future RFE but why do we even need pkill here when we can get the PID
>> of the sleep process we create and kill only that process?
>
> I thought of that too, but notice the parens "()" around that /bin/sleep;
> that creates and extra level of forked processes and its harder to get that
> pid. There probably is a way to traverse the hierarchy but I'll keep it as is
> for now.
Ah right. Begs the question why we need to use bash to execute sleep? Could it
be shell script instead of a binary?
-
PR: https://git.openjdk.java.net/jdk/pull/5239
Re: RFR: 8272600: (test) Use native "sleep" in Basic.java [v6]
On Wed, 22 Sep 2021 00:03:41 GMT, David Holmes wrote: >> I thought of that too, but notice the parens "()" around that /bin/sleep; >> that creates and extra level of forked processes and its harder to get that >> pid. There probably is a way to traverse the hierarchy but I'll keep it as >> is for now. > > Ah right. Begs the question why we need to use bash to execute sleep? Could > it be shell script instead of a binary? I think that's the author's prerogative; the test was written in 2010 to test conditions related to deeply inherited file descriptors. - PR: https://git.openjdk.java.net/jdk/pull/5239
