test: prevent workers outliving parent #9257
Conversation
mscdex
added
the
child_process
|
Thanks @gibfahn , did you trigger that yourself? I've been out of touch with node core for a while, I can't remember what the process is for triggering a PR to build in CI. |
|
Btw, I think this test is safe, in that it will fail if fork fails, and cleanup the children. It won't fix whatever might have called fork to fail, perhaps transient out of memory, or limits on number of processes, but it might help narrow down the root cause to something other than this test. And at least it won't make things worse, because after failing due to failure-to-fork, it won't leave dozens of processes around, causing fork to be even more likely to fail. |
|
@sam-github Yes I did, you just go to the Jenkins job ( |
|
LGTM provided CI is ok. |
|
parallel/test-dgram-send-empty-array failed on freebsd, I can't see how that is related. arm-fanned died with |
|
ci 3: https://ci.nodejs.org/job/node-test-pull-request/4652/ Seems to be getting worse, how do I compare to expected platform build results? |
|
@sam-github Looking at the failures in other recent PRs usually helps. I'm not sure there's an expected version to compare against, a lot of these failures are intermittent. Unless your test is doing something really crazy, any failures that aren't |
|
The test failures are unrelated to the test I modified. I'll merge this tomorrow. |
|
@cjihrig Was that question for me? I don't know - you certainly know more about node's CI setup than I do! |
|
It was more of a question for anyone listening. We do have a stress test CI job, but someone with an account on the machine would have to log in to check for leftover processes. Alternatively, someone with an AIX machine could run |
|
@cjihrig Will do both (community and our own machines) tomorrow |
|
@georgeadams95 has run the test a few hundred times on one of our AIX boxes, and there were no child processes left behind. |
test-child-process-pass-fd.js parent can exit with an error on failure to fork, in which case it will leak child processes. Limit child lifetime to that of parent. Fixes: nodejs#9255 PR-URL: nodejs#9257 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
sam-github
force-pushed
the
cp-test-die-on-loss-of-parent
branch
from
638b768 to
4d896c4
Compare
test-child-process-pass-fd.js parent can exit with an error on failure to fork, in which case it will leak child processes. Limit child lifetime to that of parent. Fixes: #9255 PR-URL: #9257 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test-child-process-pass-fd.js parent can exit with an error on failure to fork, in which case it will leak child processes. Limit child lifetime to that of parent. Fixes: #9255 PR-URL: #9257 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test-child-process-pass-fd.js parent can exit with an error on failure to fork, in which case it will leak child processes. Limit child lifetime to that of parent. Fixes: #9255 PR-URL: #9257 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
@MylesBorins why don't land on v4? |
|
I see, it conflicts, I can backport, do we care? The reason to backport would be for other PRs to land better, but it doesn't seem to have been a problem. Also, for ci stability. |
|
@sam-github I think CI stability is a compelling reason to backport. |
|
@MylesBorins if you will accept, I will backport, but I need some encouragement, I don't want to waste my time |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesWaiting for confirmation on CI, which was leaking child processes.
Affected core subsystem(s)
test
Description of change
test-child-process-pass-fd.js parent can exit with an error on failure
to fork, in which case it will leak child processes. Limit child
lifetime to that of parent.
Fix #9255
/to @mhdawson