Hi Thomas,
This is a change that should have a release note, can you fill it in:
https://bugs.openjdk.java.net/browse/JDK-8218924
Also, I had to do some manual updates to the issues to get the changeset
into the history.
The issue number should have been 8213192 in the summary lineand on the
emails.
Hgupdater watches the commits and uses the bugid to update the bug;
wrong bug, wrong updates.
Thanks, Roger
On 2/12/19 1:41 AM, Thomas Stüfe wrote:
On Mon, Feb 11, 2019 at 9:18 PM Martin Buchholz <marti...@google.com
<mailto:marti...@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.stu...@gmail.com <mailto:thomas.stu...@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.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
- 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
@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
(@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
