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
Launcher: force kill chrome in case of working with temporary profile. #693
Conversation
This patch: - in case of user-supplied profile directory, starts gracefully closing chrome so that it doesn't corrupt profile - in case of temp profile directory, force-closes chrome and proactively tries to remove temp directory to avoid littering Fixes puppeteer#527.
@JoelEinbinder could you please check how this behaves on Windows? |
process.kill(-chromeProcess.pid, 'SIGKILL'); | ||
// Attempt to remove temporary profile directory to avoid littering. | ||
try { | ||
removeSync(temporaryUserDataDir); |
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.
shouldn't the 'close' event handle this? Or we try just in case the close never happens?
} else { | ||
// Terminate chrome gracefully. | ||
if (process.platform === 'win32') | ||
childProcess.execSync(`taskkill /pid ${chromeProcess.pid} /T`); |
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.
This didn't work for me on Windows.
This process can only be terminated forcefully (with /F option)
I'll send a PR with what worked for me.
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.
It doesn't work :(
Closing in favor of #700 |
This patch:
chrome so that it doesn't corrupt profile
tries to remove temp directory to avoid littering
Fixes #527.