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

[Thomas Stüfe]

Hi Roger,

I start getting doubts about the whole test, honestly. Do you think it is
worth the maintenance effort to test that we indeed call posix_spawn as we
intended to? I wonder whether I should just scratch it.

..Thomas



On Fri, Feb 8, 2019 at 10:00 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:

> 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
>
> - 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>
> 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
>>
>> @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>
>> 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
>>>
>>> (@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
>>>
>>>
>