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

Fri, 08 Feb 2019 13:03:24 -0800 

Hi Thomas,

The new OutputAnalyzer methods are new:
496: now that we have Immutable Lists, can the filterByKeyword argument be a List<String>?
List<String> interesting = List.of( "fork", "clone", "vfork", "exec", "/bin/true", "jspawnhelper" ); 

497: Instead of passing an int, pass the String itself
480, 485: that is returned from getStdout() or getStderr().
The body of the method could probably be nicely written using the Stream API. (except for the line numbers).  I'm not sure the line numbers are useful in this case 
based since the strace output is not shown anywhere or seen in the full.

   <string>.lines().filter()...
Add the javadoc to the methods so its clear how to use them since this is a public test framework. 

Regards, Roger


On 02/08/2019 04:38 AM, Thomas Stüfe 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

Reply via email to