Fix childprocess killing when the parent process SIGINTs #6011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If you `ctrl + c` the Puppeteer parent process, we would sometimes not properly handle killing of the child processes. This would then leave child processes behind, with running Chromium instances. This in turn could block Puppeteer from launching again and results in cryptic errors. Instead of using the generic `process.kill` with the process id (which for some reason is negative the pid, which I don't get), we can kill the child process directly by calling `proc.kill`.
|
This is an attempt to fix the pesky " |
|
Seems like there is only 1 test failing. That seems hopeful. There are some failures on Windows though. But I suppose we can use the same |
|
The test failures look like a simple error message has changed too which is great. Could we write a test that demonstrates this new behaviour? |
On Windows, it might not always cleanup the process, as it was already cleaned up. We can relax the check here.
mocha-config/base.js
Outdated
| @@ -1,4 +1,6 @@ | |||
| module.exports = { | |||
| reporter: 'dot', | |||
| // Allow `console.logs` to show up during test execution | |||
There was a problem hiding this comment.
nit:
Suggested change
| // Allow `console.logs` to show up during test execution | |
| // Allow `console.log`s to show up during test execution |
src/launcher/BrowserRunner.ts
Outdated
Comment on lines
156
to
157
| throw new Error(`${PROCESS_ERROR_EXPLANATION} | ||
| Error cause: ${error.stack}`); |
There was a problem hiding this comment.
nit to maintain indentation:
Suggested change
| throw new Error(`${PROCESS_ERROR_EXPLANATION} | |
| Error cause: ${error.stack}`); | |
| throw new Error(`${PROCESS_ERROR_EXPLANATION}\nError cause: ${error.stack}`); |
mathiasbynens
approved these changes
Jun 15, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If you
ctrl + cthe Puppeteer parent process, we would sometimesnot properly handle killing of the child processes. This would then
leave child processes behind, with running Chromium instances. This
in turn could block Puppeteer from launching again and results in
cryptic errors.
Instead of using the generic
process.killwith the process id(which for some reason is negative the pid, which I don't get),
we can kill the child process directly by calling
proc.kill.Additionally, we have to remove the eventlisteners as the last
invocation of a Node emitter callback. If we remove the listener
before the full callback is run, it would prevent further execution.
E.g. if the first call is the removal of the event listener, then the
cleanup later in the callback would not happen.
You can test this locally by running the unittests (
npm run unit)and terminating the process prematurely (
ctrl + c). Before thischange, the process would terminate with exit code 0. With this
change, it now properly terminates with exit code 130 (as defined
by the
BrowserRunner.tsin theSIGINTcallback).Lastly, I added a more helpful error message in the event that
Puppeteer is not able to clean up the Node process. In theory,
this should now no longer happen. However, if it does, we now
explain what is going on and where users can report their bug.
After this change, whenever you run the unit tests or anything
related to Puppeteer, you should have zero browser processes
lingering. E.g.
ps aux | grep Chromiumshould show noresults. This therefore prevents the need for continuous killing
of the browser processes when working on Puppeteer.
Fixes #5729
Fixes #4796
Fixes #4963
Fixes #4333
Fixes #1825