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

[Roger Riggs]

Great!  Looks fine.

Thanks, Roger

On 02/11/2019 01:50 PM, Thomas Stüfe wrote:
> Hi Roger, Martin,
>
> hopefully final version:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/ 
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/>
>
> I removed the test and the changes in the test library made for the 
> test. Test is just too brittle with too little nourishing value. 
> Everything else is unchanged from webrev.02.
>
> Thank you, Thomas
>
>
>
> On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
>     Hi Roger, Martin,
>
>     next version:
>
>     http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev
>     <http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev>
>
>     - did massage the comment in ProcessImpl.c
>     - made the test more resilient by scanning for the strace tool and
>     by silently ignoring all problems stemming from strace or the
>     payload binary not being there. The test now only fails if the
>     forks were successully executed but it does not seem to use
>     posix-spawn.
>     - added output to the test by printing the "interesting" lines of
>     the strace output. Note that this filtering is not really
>     sophisticated and will show all thread related clone() calls as well:
>
>     614   [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0,
>     flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>     parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700,
>     child_tidptr=0x7fe00c4bb9d0) = 12452
>     646   [pid 12447] clone(/usr/bin/strace: Process 12453 attached
>     649   [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0,
>     flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
>     parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700,
>     child_tidptr=0x7fe00c3ba9d0) = 12453
>     ....
>
>     I am sure this could be made more intelligent but lets keep it
>     simple for now.
>
>     - I removed the helperPath() methods Roger mentioned, they are not
>     needed anymore.
>
>     @Martin: I like the -e signal=none -e trace=process idea but I'm
>     not sure if all versions of strace support these options. I think
>     the strace output is small enough for this small use case, some kB
>     only.
>
>     Cheers, Thomas
>
>
>
>
>
>     On Thu, Feb 7, 2019 at 6:01 PM 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
>