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

[Thomas Stüfe]

On Mon, Feb 11, 2019 at 9:18 PM Martin Buchholz <martinrb at google.com> wrote:

> Looks good to me.
>
>
Thank you, I just pushed.


> It's true that these tests depending on external tools are very brittle.
> In particular, strace is in the middle of a flag re-org
>
>        -e trace=%process
>        -e trace=process (deprecated)
>
> Nevertheless, we have such tests - are they worth the maintenance burden?
> My own Zombies.java test is a good example.
>

It is more useful than my proposed test was. I wince a bit at the perl
requirement though. Especially since the test silently quits if no perl is
installed.

(As a side note, I wonder whether we could have a mechanism to signal
requirements not met, eg. with a TestRequirementsNotMetException, and then
let the test executor decide what to do: warn, ignore, error...)

I guess part of this could be redone nowadays with Rogers ProcessHandle API
(the child seeking), but we still would need to find out if the child is a
zombie.

I like the test name btw. Very succinct :)

..Thomas



>
> On Mon, Feb 11, 2019 at 10:50 AM Thomas Stüfe <thomas.stuefe at gmail.com>
> 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/
>>
>> 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>
>> 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
>>>>>
>>>>>