test: cover thrown errors from exec() kill #11038
Conversation
nodejs-github-bot
added
test
| // Keep the process alive and printing to stdout. | ||
| setInterval(() => { console.log('foo'); }, 1); | ||
| } else { | ||
| // Monkey path ChildProcess#kill() to kill the process and then throw. |
| const cmd = `${process.execPath} ${__filename} child`; | ||
| const options = { maxBuffer: 0 }; | ||
| const child = cp.exec(cmd, options, common.mustCall((err, stdout, stderr) => { | ||
| // Verify that if ChildProcess#kill() throws, the error is reported. |
There was a problem hiding this comment.
what if it throws because the child exited, wouldn't that be success?
There was a problem hiding this comment.
I'm not sure that I understand the question. Do you mean if Node decides to kill the child process, but in the interim it exits on its own? If exithandler() runs (via the close handler for example), then the error won't be reported because it contains a check to make sure it doesn't run twice.
FWIW, this is the code path being tested.
There was a problem hiding this comment.
node could decided to kill the child, but between when it decided and the child is killed, the child can exit. It would be possible to special case
Line 265 in 3268863
ESRCH, and not call exit handler in that case, because it just means that node has not yet epolled the exit status, but it will in the next tick or so. But the event ordering of the timeout vs the process exit is indeterminate by nature, so its no big deal.
This commit adds code coverage for the scenario where exec() kills a child process, but the call to ChildProcess#kill() throws an exception. PR-URL: nodejs#11038 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
CI was 💚 |
|
It's been sitting for a bit, new CI: https://ci.nodejs.org/job/node-test-pull-request/6461/ |
|
Landed in 9ac363b |
This commit adds code coverage for the scenario where exec() kills a child process, but the call to ChildProcess#kill() throws an exception. PR-URL: #11038 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
|
We'll have to watch this test carefully. I hadn't landed this yet because it appeared to be flakey on Windows. |
|
Stress test on Windows: https://ci.nodejs.org/job/node-stress-single-test/1141/ |
|
Stress test results: @cjihrig should we revert? Mark flaky on windows? |
|
I'd be OK with either. I think there might be a genuine bug with |
PR-URL: nodejs#12054 Ref: nodejs#12053 Ref: nodejs#11038 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
This commit adds code coverage for the scenario where exec() kills a child process, but the call to ChildProcess#kill() throws an exception. PR-URL: #11038 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins
added
the
baking-for-lts
MylesBorins
removed
the
baking-for-lts
This commit adds code coverage for the scenario where
exec()kills a child process, but the call toChildProcess#kill()throws an exception.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test