Sorry, Roger, I must have messed up the bug id when pushing. Thank you for fixing!
This would be a good thing to have a jcheck for. ..Thomas On Wed, Feb 13, 2019 at 4:56 PM Roger Riggs <roger.ri...@oracle.com> wrote: > 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> > 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> >> 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> >>> 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> >>>> 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> >>>>> 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 >>>>>> >>>>>> >
