RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn

[Roger Riggs]

Hi,

I agree with Martin's editorial comments.

I've had an eye on the helperPath methods and they could be removed;
the path is no longer architecture specific and javahome is available 
statically;
So the methods can be removed and helperpath reduces to:

172     private static final byte[] helperpath = toCString(StaticProperty.javaHome() + /lib/jspawnhelper");

If you think that's in scope, please apply; otherwise I'll fix it later.

Thanks, Roger

On 02/07/2019 01:09 PM, Martin Buchholz wrote:
> This is very good.  Approved.
> But as always I have review suggestions
>
> typos:
> ithe
> Preceede => Precede
>
> Drop "the"
> How does the glibc implement posix_spawn?
> How does the muslc implement posix_spawn?
>
> parents => the parent
> CLONE_VFORK means parents waits until we exec, as with (2)
>
> an own => a separate
> we pass an own stack for the child to run on
> Did you mean tlrd => TL;DR ? https://en.wikipedia.org/wiki/TL;DR
> You can drop the pre-2004 part from the TL;DR
> 164  * tlrd: calling posix_spawn(3) for glibc
>  165  * (1) < 2.4 would expose us to memory overcommit problems. But 
> this glibc is
> The test is still too brittle for my taste.
> I would check  for the existence of /usr/bin/strace (and /bin/true !) 
> and quietly skip the test if not found.
>
> I don't like uninformative prints
>         System.out.print("I'm the child. Will fork /bin/true now...");
> Instead I might be truly interested in whether the strace output 
> contains fork|vfork|clone
>
> fyi I have a wrapper around strace for process-related syscalls
>
> #!/bin/bash
> /usr/bin/strace -f -v -s 256 -e signal=none -e trace=process "$@"
>
>
>
>
> On Thu, Feb 7, 2019 at 9:01 AM Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
>     Hi all,
>
>     second version, including the updated comment in ProcessImpl.c
>     Martin requested:
>
>     http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html
>     <http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html>
>
>     @Roger: thanks for feeding this into your tests. I still try to
>     get it to run thru jdk-submit, but that seems to be stuck again..
>
>     Cheers, Thomas
>
>
>
>
>
>     On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe
>     <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>> wrote:
>
>         Hi all
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8213192
>         webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html>
>
>         (@Roger: I hope you do not mind? The bug is assigned to you
>         but since I happened to play around with posix_spawn I
>         prepared this webrev. If you rather do this change, that is
>         fine and I will leave it to you.)
>
>         When we added the possibility to use posix_spawn as underlying
>         implementation for Runtime.exec() on Linux with
>         https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to
>         keep VFORK as default until work on 13 starts. So now would be
>         a good time to switch the default to posix_spawn to get a good
>         testing window. Note that at SAP we run our VMs internally
>         with posix_spawn as default since some months and have not
>         seen problems.
>
>         As for the fix, I added a test which tests that the default is
>         indeed posix_spawn - not sure whether this is overdoing it
>         though. Also, I use strace for the test, and /bin/true, and
>         while strace is usually available and reachable by path
>         resolution, I am afraid on some test machines it may not. What
>         do you think, should I leave the test out?
>
>         The fix ran through all java/lang/ProcessBuilder jtreg tests ok.
>
>         Thanks, Thomas
>