Hi all, Basic considerations:
This patch enables an untested function to the end user. I am unsure about this. I would feel better if we had something like "-XX:+UnlockDiagnosticVMOptions" for the JDK. That said, I can see the merit in having this switch to play around with. It would it make easier for us to test posix_spawn() (for us, the next step would be to enable it by default in our nightly tests at SAP and just run the nightlies with it for a couple of days or weeks). So, bottomline, I have no strong reservations if no one else has.+ --- Review: - copyright dates need updating on the C-sources - I opt for "#if defined(__solaris__) || defined(_ALLBSD_SOURCE) || defined(_AIX) || defined(__linux__)" to be removed completely from unix-specific source files. The ifdef now covers all OpenJDK Unix platforms. Looks fine otherwise. --- Best, Thomas On Wed, Oct 24, 2018 at 7:51 AM Thomas Stüfe <thomas.stu...@gmail.com> wrote: > > For the convenience of the reviewers, here webrev and bug: > > https://bugs.openjdk.java.net/browse/JDK-8212828 > http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8212828-posix_spawn.patch/webrev/ > > submit tests are currently running. > > ..Thomas > > On Tue, Oct 23, 2018 at 9:27 PM David Lloyd <david.ll...@redhat.com> wrote: > > > > My plans to try jdk/submit have fallen through unfortunately, as I > > cannot seem to gain direct or indirect access to that system. So I > > guess I'm looking for any reviews on this patch now. Thomas has > > volunteered to sponsor. > > > > Thanks. > > > > On Tue, Oct 23, 2018 at 10:49 AM Thomas Stüfe <thomas.stu...@gmail.com> > > wrote: > > > > > > Here you go: > > > > > > https://bugs.openjdk.java.net/browse/JDK-8212828 > > > > > > If noone else steps in, I can sponsor the change for you. > > > > > > Cheers, Thomas > > > On Tue, Oct 23, 2018 at 4:19 PM David Lloyd <david.ll...@redhat.com> > > > wrote: > > > > > > > > Sure. I don't have any tracking information on the bugreport one I > > > > submitted, but if you can track that down and promote it, it would > > > > save you some typing. Otherwise whatever you can do would be great, > > > > thanks. > > > > On Tue, Oct 23, 2018 at 9:02 AM Thomas Stüfe <thomas.stu...@gmail.com> > > > > wrote: > > > > > > > > > > Oh, I can open a bug report on JBS for you. Should I? > > > > > > > > > > (Now I understand the "reuse bug id"). > > > > > > > > > > > > > > > On Tue, Oct 23, 2018 at 3:18 PM David Lloyd <david.ll...@redhat.com> > > > > > wrote: > > > > > > > > > > > > I've submitted a bug report via bugreport.java.com. If/when it gets > > > > > > promoted to a proper JIRA with an issue number, I'll see if I can > > > > > > put > > > > > > the patch up on jdk/submit. > > > > > > On Thu, Oct 18, 2018 at 4:42 PM David Lloyd > > > > > > <david.ll...@redhat.com> wrote: > > > > > > > > > > > > > > The issue 6850720 isn't _exactly_ to use POSIX_SPAWN for process > > > > > > > launching on Linux, but it's the closest I could find out of what > > > > > > > are > > > > > > > really a surprisingly large number of issues that refer to > > > > > > > posix_spawn > > > > > > > in one way or another relating to ProcessImpl. There's a > > > > > > > different > > > > > > > issue to move from vfork to posix_spawn on Solaris, but I wasn't > > > > > > > sure > > > > > > > if that one was quite right to hang this off of. Maybe it should > > > > > > > be > > > > > > > yet another issue of its own. > > > > > > > > > > > > > > Anyway: this is a follow-up to the email thread entitled > > > > > > > "Runtime.exec > > > > > > > : vfork() concerns and a fix proposal", where it was casually > > > > > > > mentioned that maybe posix_spawn could become an option on Linux, > > > > > > > whereafter it could be thoroughly tested by brave individuals and > > > > > > > eventually maybe become the default on that platform, obsoleting > > > > > > > the > > > > > > > vfork support for good. > > > > > > > > > > > > > > The following patch does just that. I've tested it launching a > > > > > > > multi-process WildFly instance a bunch of times, in conjunction > > > > > > > with > > > > > > > the conveniently existent "jdk.lang.Process.launchMechanism" > > > > > > > property, > > > > > > > and nothing exploded so here it is. The usual deal with git > > > > > > > patches: > > > > > > > apply directly through "patch -p1". > > > > > > > > -- > > - DML