RFR(s): 8212828: (process) Change the Process launch mechanism default on Linux to be posix_spawn
Hi all Bug: https://bugs.openjdk.java.net/browse/JDK-8213192 webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... (@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
Hi Thomas, On 02/06/2019 04:29 AM, Thomas Stüfe wrote:
Hi all
Bug: https://bugs.openjdk.java.net/browse/JDK-8213192 webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... <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.) no problem, I hadn't gotten to it. Thanks for proposing it.
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 test is a bit quirky but should work ok. I'd leave it in until it fails and re-evaluate then.
If it fails on some systems, we can either configure them out or just skip the test if the process launch of strace fails. (Throw SkippedException).
The fix ran through all java/lang/ProcessBuilder jtreg tests ok.
I'll run the patch through the usual CI build here too. Thanks, Roger
Thanks, Thomas
Hi Roger, On Wed, Feb 6, 2019 at 4:15 PM Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Thomas,
On 02/06/2019 04:29 AM, Thomas Stüfe wrote:
Hi all
Bug: https://bugs.openjdk.java.net/browse/JDK-8213192 webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
(@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.)
no problem, I hadn't gotten to it. Thanks for proposing it.
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 test is a bit quirky but should work ok. I'd leave it in until it fails and re-evaluate then.
If it fails on some systems, we can either configure them out or just skip the test if the process launch of strace fails. (Throw SkippedException).
Yes, that makes sense.
The fix ran through all java/lang/ProcessBuilder jtreg tests ok.
I'll run the patch through the usual CI build here too.
Thanks. I'll run the test thru the submit repository too. Cheers, Thomas
Thanks, Roger
Thanks, Thomas
Hi Thomas, The CI tests here ran fine. Overall looks ok. Regards, Roger On 02/06/2019 01:16 PM, Thomas Stüfe wrote:
Hi Roger,
On Wed, Feb 6, 2019 at 4:15 PM Roger Riggs <Roger.Riggs@oracle.com <mailto:Roger.Riggs@oracle.com>> wrote:
Hi Thomas,
On 02/06/2019 04:29 AM, Thomas Stüfe wrote:
Hi all
Bug: https://bugs.openjdk.java.net/browse/JDK-8213192 webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... <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.)
no problem, I hadn't gotten to it. Thanks for proposing it.
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 test is a bit quirky but should work ok. I'd leave it in until it fails and re-evaluate then.
If it fails on some systems, we can either configure them out or just skip the test if the process launch of strace fails. (Throw SkippedException).
Yes, that makes sense.
The fix ran through all java/lang/ProcessBuilder jtreg tests ok.
I'll run the patch through the usual CI build here too.
Thanks. I'll run the test thru the submit repository too.
Cheers, Thomas
Thanks, Roger
Thanks, Thomas
Thomas, thanks for your hard work on this. We need to update my old analysis in src/java.base/unix/native/libjava/ProcessImpl_md.c You now understand the problem better than I ever did, so you are best to update it (most of the content can be taken from your analysis in JIRA).
Hi Martin, On Wed, Feb 6, 2019 at 8:18 PM Martin Buchholz <martinrb@google.com> wrote:
Thomas, thanks for your hard work on this.
We need to update my old analysis in src/java.base/unix/native/libjava/ProcessImpl_md.c You now understand the problem better than I ever did,
I very much doubt that :-)
so you are best to update it (most of the content can be taken from your analysis in JIRA).
Of course, that makes sense. I'll post an update tomorrow. Cheers, Thomas
Hi all, second version, including the updated comment in ProcessImpl.c Martin requested: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... @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@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-pro...
(@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
This is very good. Approved. But as always I have review suggestions typos: ithe Preceede => Precede Drop "the" How does the glibc implement posix_spawn? How does the muslc implement posix_spawn? parents => the parent CLONE_VFORK means parents waits until we exec, as with (2) an own => a separate we pass an own stack for the child to run on Did you mean tlrd => TL;DR ? https://en.wikipedia.org/wiki/TL;DR You can drop the pre-2004 part from the TL;DR 164 * tlrd: calling posix_spawn(3) for glibc 165 * (1) < 2.4 would expose us to memory overcommit problems. But this glibc is The test is still too brittle for my taste. I would check for the existence of /usr/bin/strace (and /bin/true !) and quietly skip the test if not found. I don't like uninformative prints System.out.print("I'm the child. Will fork /bin/true now..."); Instead I might be truly interested in whether the strace output contains fork|vfork|clone fyi I have a wrapper around strace for process-related syscalls #!/bin/bash /usr/bin/strace -f -v -s 256 -e signal=none -e trace=process "$@" On Thu, Feb 7, 2019 at 9:01 AM Thomas Stüfe <thomas.stuefe@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-pro...
@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@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-pro...
(@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
Hi, I agree with Martin's editorial comments. I've had an eye on the helperPath methods and they could be removed; the path is no longer architecture specific and javahome is available statically; So the methods can be removed and helperpath reduces to: 172 private static final byte[] helperpath = toCString(StaticProperty.javaHome() + /lib/jspawnhelper"); If you think that's in scope, please apply; otherwise I'll fix it later. Thanks, Roger On 02/07/2019 01:09 PM, Martin Buchholz wrote:
This is very good. Approved. But as always I have review suggestions
typos: ithe Preceede => Precede
Drop "the" How does the glibc implement posix_spawn? How does the muslc implement posix_spawn?
parents => the parent CLONE_VFORK means parents waits until we exec, as with (2)
an own => a separate we pass an own stack for the child to run on Did you mean tlrd => TL;DR ? https://en.wikipedia.org/wiki/TL;DR You can drop the pre-2004 part from the TL;DR 164 * tlrd: calling posix_spawn(3) for glibc 165 * (1) < 2.4 would expose us to memory overcommit problems. But this glibc is The test is still too brittle for my taste. I would check for the existence of /usr/bin/strace (and /bin/true !) and quietly skip the test if not found.
I don't like uninformative prints System.out.print("I'm the child. Will fork /bin/true now..."); Instead I might be truly interested in whether the strace output contains fork|vfork|clone
fyi I have a wrapper around strace for process-related syscalls
#!/bin/bash /usr/bin/strace -f -v -s 256 -e signal=none -e trace=process "$@"
On Thu, Feb 7, 2019 at 9:01 AM Thomas Stüfe <thomas.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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
Hi Roger, Martin, next version: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... - 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@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-pro...
@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@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-pro...
(@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
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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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
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@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-pro...
- 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@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-pro...
@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@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-pro...
(@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
Hi Thomas, I'd be fine with leaving it out. It only provides confirmation of the library call, nothing algorithmic or complex and there is no bug to verify. So yes, drop it and save the test time and maintenance. Thanks, Roger On 02/09/2019 10:24 AM, Thomas Stüfe wrote:
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@oracle.com <mailto:Roger.Riggs@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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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
Hi Roger, Martin, hopefully final version: http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... 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@gmail.com> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@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-pro...
@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@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-pro...
(@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
Great! Looks fine. Thanks, Roger On 02/11/2019 01:50 PM, Thomas Stüfe wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... <http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-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@gmail.com <mailto:thomas.stuefe@gmail.com>> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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.stuefe@gmail.com <mailto:thomas.stuefe@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-pro... <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
Thank you Roger! On Mon, Feb 11, 2019 at 7:55 PM Roger Riggs <Roger.Riggs@oracle.com> wrote:
Great! Looks fine.
Thanks, Roger
On 02/11/2019 01:50 PM, Thomas Stüfe wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
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@gmail.com> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@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-pro...
@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@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-pro...
(@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
Looks good to me. 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. On Mon, Feb 11, 2019 at 10:50 AM Thomas Stüfe <thomas.stuefe@gmail.com> wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
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@gmail.com> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@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-pro...
@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@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-pro...
(@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
On Mon, Feb 11, 2019 at 9:18 PM Martin Buchholz <martinrb@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@gmail.com> wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
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@gmail.com> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@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-pro...
@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@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-pro...
(@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
Hi Thomas, For tests that are not applicable but should be noted, a recent addition is throwing jtreg.SkippedException. [1] Adding @library /test/lib to the test header and import jtreg.SkippedException. Throw it when appropriate. See What if a test does not apply in a given situation? <http://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-apply-in-a-given-situation> http://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-apply-in-a-gi... Testng is generally preferred for new test over main tests and it has its own Skipped test mechanism. Regards, Roger On 02/12/2019 01:41 AM, Thomas Stüfe wrote:
(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 think there are two kinds of skipping: - Skipping where it just does not make sense to execute the test, e.g. on the wrong OS. That is unexciting. - Skipping where prerequisites are not met and I skip to reduce the test analysis load but actually I would like to execute the test. Those tests one may want to force-execute from time to time. Martins Zombies is a good example - you may never run it without ever realizing if you have not perl installed. But maybe this is all sloppy thinking - one could say the test is either important enough to be executed always, or it is not. In the former case all prerequisites should be installed and skipping is not the right thing to do. Thank you for the TestNG hint. I will check it out. Cheers, Thomas On Tue, Feb 12, 2019 at 3:53 PM Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Thomas,
For tests that are not applicable but should be noted, a recent addition is throwing jtreg.SkippedException. [1]
Adding @library /test/lib to the test header and import jtreg.SkippedException. Throw it when appropriate.
See What if a test does not apply in a given situation? <http://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-apply-in-a-given-situation>
http://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-apply-in-a-gi...
Testng is generally preferred for new test over main tests and it has its own Skipped test mechanism.
Regards, Roger
On 02/12/2019 01:41 AM, Thomas Stüfe wrote:
(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...)
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 <martinrb@google.com <mailto:martinrb@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@gmail.com <mailto:thomas.stuefe@gmail.com>> wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
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@gmail.com <mailto:thomas.stuefe@gmail.com>> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@gmail.com <mailto:thomas.stuefe@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-pro...
@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@gmail.com <mailto:thomas.stuefe@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-pro...
(@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
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.riggs@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 <martinrb@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@gmail.com> wrote:
Hi Roger, Martin,
hopefully final version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
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@gmail.com> wrote:
Hi Roger, Martin,
next version:
http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-pro...
- 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@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-pro...
@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@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-pro...
(@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
participants (3)
-
Martin Buchholz
-
Roger Riggs
-
Thomas Stüfe