On Tue, 21 Jan 2025 06:51:10 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>>> In ProcessImpl_md.c, we have adopted the Posix APIs and semantics for >>> process handling. I suggest removing the "unofficial" link, it does not >>> include useful information at this point and is fragile. >>> >>> The current behavior always setting SIGCHLD to SIG_DFL is/has been reliable >>> and does not depend on the behavior of the parent process. (Good) @tstuefe >>> May also be interested in reviewing this change. >> >> https://pubs.opengroup.org/onlinepubs/9799919799/ seems to indicate that the >> "default action is to ignore the signal", which is unclear to me. I think it >> means that the signal will not cause invocation of a user handler or abort >> the process, but does this also mean that wait and waited won't work? >> >> In any case, I agree with @RogerRiggs that keeping this behavior is fine. >> >>> >>> The previous sentence mentions Solaris and that should be removed at some >>> point but out of scope for this PR. > >> @tstuefe And you also agree that the link should be removed? (Which is what >> this PR is about) > > The 20+ years old link to the PASC error report I would probably remove; it > is likely outdated now. If one wanted to invest the time, one could test the > current behavior on our supported Unices. As long as that is not done, we can > live with setting SIG_DFL manually. > > I would replace it with a link to > https://pubs.opengroup.org/onlinepubs/007904875/functions/wait.html and point > to the section that says: > > [XSI] [Option Start] If the calling process has SA_NOCLDWAIT set or has > SIGCHLD set to SIG_IGN, > and the process has no unwaited-for children that were transformed into > zombie processes, > the calling thread shall block until all of the children of the process > containing > the calling thread terminate, and wait() and waitpid() shall fail and > set errno to [ECHILD]. [Option End] > > > Again, that is XSI though; I am not sure which of our Unices comply to that. > Linux does only partially, so one would have to test that behavior. > > Another question @RogerRiggs , I was surprised to see that we set the SIGCHLD > default behavior only at Java_java_lang_ProcessImpl_init. Should we also set > it in jspawnhelper before the exec? But I guess the assumption is that no > java process will fork before Java_java_lang_ProcessImpl_init, so maybe its > okay. > > > @tstuefe And you also agree that the link should be removed? (Which is > > > what this PR is about) > > > > > > The 20+ years old link to the PASC error report I would probably remove; it > > is likely outdated now. If one wanted to invest the time, one could test > > the current behavior on our supported Unices. As long as that is not done, > > we can live with setting SIG_DFL manually. > > Thanks for looking into this Thomas, much appreciated! > > I think the improvements you suggest here are perhaps streching the scope of > this particular PR, which was mostly to update links to use HTTPS. I must > also admit that I have a very limited understanding of these low-level Unix > APIs, so I'm reluctant to include material changes in this PR. > > I have filed https://bugs.openjdk.org/browse/JDK-8348183 to track a > review/update of the comment in ProcessImpl_md.c and suggest that we simply > remove the "unofficial" link for this PR. > > Would this be ok with you? Yes thank you! ------------- PR Comment: https://git.openjdk.org/jdk/pull/21633#issuecomment-2613833730
