Re: 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.stu...@gmail.com <mailto:thomas.stu...@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.stu...@gmail.com <mailto:thomas.stu...@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.stu...@gmail.com <mailto:thomas.stu...@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