[fix] Terminate chrome gracefully on Windows #876
Conversation
This patch starts using `taskkill` program on windows to gracefully terminate chrome. Fixes puppeteer#839.
lib/Launcher.js
Outdated
| // Terminate chrome gracefully. | ||
| if (process.platform === 'win32') | ||
| chromeProcess.kill(); | ||
| childProcess.execSync(`taskkill /pid ${chromeProcess.pid} >nul`); |
There was a problem hiding this comment.
I think you want /T /F on the end of this.
/f : Specifies that process(es) be forcefully terminated. This parameter is ignored for remote processes; all remote processes are forcefully terminated.
/t : Specifies to terminate all child processes along with the parent process, commonly known as a tree kill.
That's what we have in chrome-launcher, at least.
There was a problem hiding this comment.
@vsemozhetbyt does this address the dialog? now reading the issue thread it didn't seem like the /t /f combo was attempted.
There was a problem hiding this comment.
I have tried taskkill without these key and that was OK. I do not know if /f is needed since WM_CLOSE do the thing, I do not know either is /t is needed since we use >nul here because:
Chrome generates many processes. The errors are caused when an process does not acknowledge the WM_CLOSE message that TASKKILL sends. Only processes with a message loop will be able to receive the message, therefore, the processes that do not have a message loop will generate that error. Most likely, these processes are the chrome extensions and plugins.
If I get this right, these keys are used in case of WM_CLOSE do not succeed, so maybe they will not do any harm?
There was a problem hiding this comment.
I have tried this:
childProcess.execSync(`taskkill /pid ${chromeProcess.pid} /T /F >nul`);and this DO NOT PREVENT the dialog. So it seems the /F has a privilege and force-closes the browser not-gracefully (without WM_CLOSE or without waiting for all the final clean-up).
There was a problem hiding this comment.
As for /t it seems we send WM_CLOSE to the parent here, and all the subprocesses are also terminated (I've checked with process explorer), so there is no need for this key?
|
An interesting thing here is that making this change will make However I understand the usecase if you're re-using a user-data-dir. @vsemozhetbyt Have you tried |
|
Trying this: 'use strict';
const puppeteer = require('puppeteer');
(async function main() {
try {
const browser = await puppeteer.launch({
headless: false,
args: ['--restore-last-session=false'],
userDataDir: `${__dirname}/profile-dir`,
});
const page = await browser.newPage();
await page.goto('https://www.nytimes.com/');
await page.waitFor(1000);
await browser.close();
} catch (err) {
console.error(err);
}
})();I still get the dialog for the second and next runs. Maybe the graceful exit should be opt-in (or can be opt-out)? |
ah bummer. worth a shot. @vsemozhetbyt can you measure the time it takes to kill the process these two ways? i'd load up the browser with a few heavy tabs. @aslushnikov if you dont care either way, feel free to ignore me and merge :) |
|
@paulirish Let me know if I should measure in other way: 'use strict';
const puppeteer = require('puppeteer');
(async function main() {
try {
const browser = await puppeteer.launch({
headless: false,
userDataDir: `${__dirname}/profile-dir`,
});
const page1 = await browser.newPage();
await page1.goto('https://www.nytimes.com/');
const page2 = await browser.newPage();
await page2.goto('http://www.bbc.com/');
const page3 = await browser.newPage();
await page3.goto('http://www.dw.com/');
await page1.waitFor(1000);
console.time('test');
await browser.close();
console.timeEnd('test');
} catch (err) {
console.error(err);
}
})();Without this fix, 3 lanches: 166.242ms, 121.525ms, 153.104msWith this fix, 3 lanches: 1386.207ms, 1179.338ms, 1126.386ms |
|
FWIW, I have tried this: childProcess.execFileSync('taskkill', ['/pid', `${chromeProcess.pid}`]);as 1231.945ms, 1062.428ms, 1099.981msBTW, I do not get any errors without |
|
@paulirish we slow down only for the custom userDataDir argument, this doesn't affect the general case with a temp profile. The slow down itself makes sense to me: we ask chrome to finish its business before closing, and it takes some time in doing so. @vsemozhetbyt thanks, I removed the redirect. |
This patch starts using
taskkillprogram on windows to gracefullyterminate chrome.
Fixes #839.