Hi Roger, On Tue, Oct 30, 2018 at 3:46 PM Roger Riggs <Roger.Riggs@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@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/... 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@gmail.com> wrote:
Hi Roger,
On Wed 24. Oct 2018 at 21:39, Roger Riggs <roger.riggs@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@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/web...
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@redhat.com> wrote:
On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe <thomas.stuefe@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