-
Notifications
You must be signed in to change notification settings - Fork 9.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Kill browser process when killing process group fails #8477
Conversation
Fixes puppeteer#8476. Similar fix to puppeteer#8352 which only covered Windows.
@@ -195,15 +195,23 @@ export class BrowserRunner { | |||
if (error) { | |||
// taskkill can fail to kill the process e.g. due to missing permissions. | |||
// Let's kill the process via Node API. This delays killing of all child | |||
// proccesses of `this.proc` until the main Node.js process dies. | |||
// processes of `this.proc` until the main Node.js process dies. | |||
proc.kill(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimmydief is SIGKILL
missing there in an intentional way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kikobeats I had wondered the same. FYI @szuend who made the original change in #8352.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIGKILL
is not a thing on windows. So if I remember the Node.js documentation correctly a call to kill
without any signal is equivalent to SIGKILL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, thanks for clarifying.
I considered the kill logic enough important to be alive as a separate package:
http://npm.im/kill-process-group
I was wondering if puppeteer will accept it if I send a PR; happy to invite anyone interested as contributor of the repo.
What kind of change does this PR introduce?
Bug fix.
Did you add tests for your changes?
I did not add any – seems like this area is missing tests in general. I'm having trouble finding a simplified reproduction of this outside of my private project.
If relevant, did you update the documentation?
N/A
Summary
Fixes #8476 which seems to have been a regression from #6859. Similar fix to #8352 which only covered Windows.
Does this PR introduce a breaking change?
No.
Other information