RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

Tue Oct 30 14:46:27 UTC 2018

Hi Thomas,

On 10/29/2018 12:04 PM, Thomas Stüfe wrote:
> Hi Roger,
>
> On Thu, Oct 25, 2018 at 10:45 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>> Hi Thomas,
>>
>> In an abundance of caution, I was thinking that it would be a change right
>> at the beginning of a new release so it gets the most exercise and
>> users in early access, etc.
>>
> Okay, I understand that.
>
> Over the next days I will run tests with posix_spawn enabled by
> default on our landscape. We have many different Linuxes on different
> architectures and different levels of glibc, so this is a reasonable
> test.
>
> If I do not encounter red flags, I would consider the posix_spawn path
> tested well enough to ship it at least as a non-default, experimental
> option. Like David originally intended. Then, start of next release,
> we can make it default and see how that goes.
>
> Does that sound ok to you?

That's fine, until it becomes the default, it will be opt in.

Is there an updated webrev with the corrected test executions?
>
>> And before that it needs to be put into more regular usage and
>> some more unusual environments. The default is currently selected
>> by virtual of being first in the argument list; that doesn't lend itself
>> to being configurable with either configure or make arguments.
> I do not understand the last sentence: it was never my intention of
> adding a configure option?
I was considering whether it would be useful to have such an option to
make it easier to make it the default in a particular build.
But it seems unnecessary since the tests can be done without.

Thanks, Roger

...
>
>> On 10/25/2018 06:33 AM, Thomas Stüfe wrote:
>>
>> Hi all,
>>
>> the more I mull over this, the more I would prefer to do the jump for
>> real and attempt switch the default to posix_spawn() for Linux.
>>
>> We have theoretically established that both glibc down to 2.4 and
>> muslc since always did "the right thing".
>>
>> We still have time in the 12 time line to test this thoroughly.
>>
>> What do you think?
>>
>> Thanks, Thomas
>>
>> On Wed, Oct 24, 2018 at 10:59 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>> Hi Roger,
>>
>> On Wed 24. Oct 2018 at 21:39, Roger Riggs <roger.riggs at oracle.com> wrote:
>>
>> Hi Thomas,
>>
>> Sorry, I had the incantation for multiple tests wrong.
>> Separate test configurations need to be in separate /* */ comment blocks.
>> BTW, it creates separate .jtr files for each block.
>>
>> diff --git a/test/jdk/java/lang/ProcessBuilder/Basic.java b/test/jdk/java/lang/ProcessBuilder/Basic.java
>> --- a/test/jdk/java/lang/ProcessBuilder/Basic.java
>> +++ b/test/jdk/java/lang/ProcessBuilder/Basic.java
>> @@ -33,8 +33,10 @@
>>    * @modules java.base/java.lang:open
>>    * @run main/othervm/timeout=300 Basic
>>    * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork Basic
>> - *
>> + */
>> +/*
>>    * @test
>> + * @modules java.base/java.lang:open
>>    * @requires (os.family == "linux")
>>    * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>    * @author Martin Buchholz
>>
>>
>> As for the build configuration being out of scope...
>>
>> I don't want to see part of the work done and then the rest forgotten or
>> worse yet someone comes along in the future and incorrectly believes that
>> posix_spawn is ready for production and starts filing bugs or getting bit by something.
>> There's a lot more testing to do before it could be come the default
>> and perhaps a significant warning should be attached to the code so it does not get forgotten.
>>
>> I agree, we ideally should not roll out half tested changes. But where does that leave us wrt this patch?
>>
>> We could just not do it, requiring anyone willing to do the extensive testing necessary to switch the default to posix-spawn to apply this change first locally. This is not an insurmountable amount of work, especially since the base is quite static, so the patch won’t bit rot easily.
>>
>> We could push the patch in its current form, plus a large source comment? But then. Users do not read comments. A warning on stderr? Would play havoc with many tests parsing stderr.
>>
>> Do you have an idea how to proceed?
>>
>> Thanks, Thomas
>>
>> Regards, Roger
>>
>>
>> On 10/24/18 1:53 PM, Thomas Stüfe wrote:
>>
>> Hi Roger,
>>
>> On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>
>> Hi Thomas,
>>
>> Thanks for adding the test.
>>
>> There's a feature of jtreg that was exposed a couple of month ago that
>> can be used to deal with running the test too many times.
>>
>> There can be multiple @test blocks with different @requires.
>>
>> * @run main/othervm/timeout=300 Basic * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=fork Basic * * @test * @requires
>> (os.family == "linux") * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>
>>
>> Does not seem to work, sorry:
>>
>>    24 /*
>>    25  * @test
>>    26  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 4986689
>>    27  *      5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
>>    28  *      6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
>>    29  *      4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
>>    30  *      8067796
>>    31  * @key intermittent
>>    32  * @summary Basic tests for Process and Environment Variable code
>>    33  * @modules java.base/java.lang:open
>>    34  * @run main/othervm/timeout=300 Basic
>>    35  * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=fork Basic
>>    36  * @test
>>    37  * @requires (os.family == "liinux")
>>    38  * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>    39  * @author Martin Buchholz
>>    40  */
>>
>> If I have @requires (os.family == "linux") all three variants are executed - ok.
>>
>> Then I wanted to have a negative test, so I misnamed linux as "liinux"
>> and would have expected the first @test block to fire, the second to
>> be ignored. But in fact:
>>
>> thomas at mainframe /shared/projects/openjdk/jtreg $
>> ../jtreg-prebuilt/jtreg/bin/jtreg -jdk
>> ../jdk-jdk/output-slowdebug/images/jdk/
>> ../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
>> Test results: no tests selected
>>
>> So to me it looks like as if the @requires tag is valid for both @test
>> blocks, not only the one it appears in.
>>
>> I'll run this through our build system too.
>>
>> To fully test out using posix_spawn in many more different scenarios the
>> build system should be augmented to be able to use it as the default for
>> an entire build/CI/CD/test runs.
>>
>> Sure! But I think this is out of scope for this patch.
>>
>> Thanks, Thomas
>>
>> Thanks, Roger
>>
>> On 10/24/2018 10:35 AM, Thomas Stüfe wrote:
>>
>> Hi all,
>>
>> version 2 of Davids patch, with test changes:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8212828-posix_spawn.patch/webrev.01/webrev/
>>
>> Executed the test on my local Ubuntu box, works fine. Submit job runs.
>>
>> About the test: I added a new line:
>>
>>     * @run main/othervm/timeout=300 Basic
>>     * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork Basic
>> + * @run main/othervm/timeout=300
>> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>>
>> The way I understand those tests work is that the first line causes
>> the test to be run with the default launch mechanism, the second line
>> with jdk.lang.Process.launchMechanism=fork explicitly. The third line,
>> added by me, will now explicitly test with posix_spawn. I did not
>> limit the platforms, since posix_spawn should now be available on all
>> platforms, so it should work on all platforms. But then, on all other
>> platforms it has already been the default.
>>
>> This is still a bit iffy: On Windows, jdk.lang.Process.launchMechanism
>> gets ignored, so we just executed the same test twice (now three
>> times)? And now we execute the posix_spawn variant twice on all
>> platforms where this is the default, so line 1 and 3 are the same? You
>> see I am not a jtreg expert :) Can I specify a @run directive for only
>> one platform? In that case I would limit the explicit posix_spawn test
>> to Linux.
>>
>> Note however that if we really abondon vfork in the future and make
>> posix_spawn the default, this test becomes simpler too.
>>
>> Best, Thomas
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> On Wed, Oct 24, 2018 at 3:36 PM David Lloyd <david.lloyd at redhat.com> wrote:
>>
>> On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>> Review:
>>
>> - copyright dates need updating on the C-sources
>>
>> - I opt for "#if defined(__solaris__) || defined(_ALLBSD_SOURCE) ||
>> defined(_AIX) || defined(__linux__)" to be removed completely from
>> unix-specific source files. The ifdef now covers all OpenJDK Unix
>> platforms.
>>
>> Here's a version with these changes.
>>
>> --
>> - DML
>>
>>
>>