RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux
Roger Riggs
roger.riggs at oracle.com
Wed Oct 24 19:39:07 UTC 2018
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.
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
More information about the core-libs-dev
mailing list