Unreliable process killing on windows

[paulirish]

There are several issues about this problem. (in this repo and in ligththouse's)

I'm going to jot down some notes here.

We use taskkill to kill the chrome process on windows:

chrome-launcher/src/chrome-launcher.ts

Line 388 in 279577f

execSync(`taskkill /pid ${this.chrome.pid} /T /F`, {stdio: 'pipe'});

We have intermittent failures where this command fails and thus launcher.kill() rejects. The failures look like:

Error: Couldn't quit Chrome process. 
Error: Chrome could not be killed Command failed: taskkill /pid 13216 /T /F
ERROR: The process with PID 8536 (child process of PID 13216) could not be terminated.
Reason: The operation attempted is not supported.

History of related changes:

• fix: log taskkill error based on logging opts (#178) #179 made sure stdio from the taskkill cmd is piped up but not emitted.
• fix: reject promise on failed kill #112 added the rejection, primarily for the sigINT usecase?

[paulirish]

chrome-launcher kills like this:

chrome-launcher/src/chrome-launcher.ts

Lines 375 to 403 in 279577f

	kill() {
	return new Promise<void>((resolve, reject) => {
	if (this.chrome) {
	this.chrome.on('close', () => {
	delete this.chrome;
	this.destroyTmp().then(resolve);
	});
	log.log('ChromeLauncher', `Killing Chrome instance ${this.chrome.pid}`);
	try {
	if (isWindows) {
	// While pipe is the default, stderr also gets printed to process.stderr
	// if you don't explicitly set `stdio`
	execSync(`taskkill /pid ${this.chrome.pid} /T /F`, {stdio: 'pipe'});
	} else {
	if (this.chrome.pid) {
	process.kill(-this.chrome.pid);
	}
	}
	} catch (err) {
	const message = `Chrome could not be killed ${err.message}`;
	log.warn('ChromeLauncher', message);
	reject(new Error(message));
	}
	} else {
	// fail silently as we did not start chrome
	resolve();
	}
	});

pptr kills like this:

https://github.com/puppeteer/puppeteer/blob/256223a7b18672ae3890bde312986482ea0fed52/src/node/BrowserRunner.ts#L182-L209

in fact the if (error) this.proc.kill() was added two weeks ago.

playwright kills like this:

https://github.com/microsoft/playwright/blob/634ba85c83ae19de1e1610e5b393e35eb03012e4/packages/playwright-core/src/utils/processLauncher.ts#L165-L189

playwright had another fix for the process xxxx not found but removed it somewhat recently.

---

both pptr and playwright use the Browser.close method as a graceful close before attempting this process kill approach. as it's CDP, it's hard to introduce to chrome-launcher which doesn't address any CDP stuff at all.

[paulirish]

I charted out playwrights complete flow in pseudo cuz i was pretty curious.

it's interesting, though i'm not sure how deliberate it is at this point:

async function closeOrKill() {
  try {
    await Promise.race([gracefullyClose(), deadlinePromise]);
  } catch (e) {
    await killProcessAndCleanup().catch(_ => {});
  }
}

spawnedProcess.once('exit', (exitCode, signal) => {
  removeEventListeners();
  cleanupTmpDir();
});

process.on('exit', killProcessAndCleanup);
onSigInt(async _ => {
  await gracefullyClose();
  process.exit(130);
});

async function gracefullyClose() {
  if (alreadyGracefullyClosing) {
    await killProcess();
    await cleanupTmpDir();
    return;
  }
  await cdp.send('Browser.close').catch(_ => killProcess());
  await cleanupTmpDir();
}

function killProcess() {
  removeEventListeners();
  if (process.pid & !killed && !closed) {
    try {
      if (win32) {
        spawnSync('taskkill /t /f ... ');
      } else {
        process.kill(-pid, 'sigkill');
      }
    } catch (e) {
      log('process already done');
    }
  } else {
    log('already done i guess');
  }
}

function killProcessAndCleanup() {
  killProcess();
  cleanupTmpDir();
}

function cleanupTmpDir() {
  rimraf(userdatadir);
}

[paulirish]

Last things worth comparing:

should chrome process be launched as detached?

• chrome-launcher: yeah we use detached:true
• both pptr and playwright: detached:true for non-windows. detached:false for windows:

https://github.com/puppeteer/puppeteer/blob/256223a7b18672ae3890bde312986482ea0fed52/src/node/BrowserRunner.ts#L93-L97

https://github.com/microsoft/playwright/blob/634ba85c83ae19de1e1610e5b393e35eb03012e4/packages/playwright-core/src/utils/processLauncher.ts#L69-L72

---

do you resolve or reject if the taskkill cmd fails?

• chrome-launcher does now, but it was changed to that years ago, for curious reasons.
• both pptr and pw do not (and did not) reject.

[brendankenny]

Another example in GoogleChrome/lighthouse#14376