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

[Roger Riggs]

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.ri...@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.stu...@gmail.com> wrote:

Hi Roger,

On Wed 24. Oct 2018 at 21:39, Roger Riggs <roger.ri...@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.ri...@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@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.ll...@redhat.com> wrote:

On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe <thomas.stu...@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