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

Tue Oct 30 15:41:56 UTC 2018

Hi Roger,

On Tue, Oct 30, 2018 at 3:46 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>
> 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?
>

Here you go.

http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8212828-posix_spawn-on-linux/webrev_0/webrev/

Tested locally, seems to work fine. I am re-running the submit tests too.

> 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.
>

Okay, understood, thanks for clarifying.

Thanks, Thomas

> 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
>
>
>
>