Consider using `taskkill.exe` on Windows to gracefully terminate processes

[meganrogge]

Follow-up from microsoft/vscode#248947

I have found that with conptyDll enabled, terminal processes are not killed gracefully - for example, addresses in use remain blocked when I kill a terminal and rerun a server.

node-pty/src/windowsPtyAgent.ts

Lines 177 to 184 in d738123

	} else {
	// Close the input write handle to signal the end of session.
	this._inSocket.destroy();
	(this._ptyNative as IConptyNative).kill(this._pty, this._useConptyDll);
	this._outSocket.on('data', () => {
	this._conoutSocketWorker.dispose();
	});
	}

Using taskkill.exe seems to work both for when conptyDll is enabled and disabled

https://github.com/microsoft/vscode/pull/248947/files#diff-7fa6408c8e509843aa68feb54ddc0dc812a3d8cbfc5e6df7e5ed25d90978c082R396-R402

We should consider moving this from vscode into node-pty.

[deepak1556]

Can you share the test case that was used to verify this behavior, it would help me to understand the issue. Thanks!

[meganrogge]

Yes - open xterm.js repo. Run the build task - it'll start the demo server. Kill the terminals. Restart the task (with this graceful kill setting disabled). It'll error saying address is still in use. Clear the port.

Enable the setting. Repeat the steps. It will rerun the task successfully - no address in use error.

[deepak1556]

Would be good to isolate a minimal case with node-pty, we have an existing test case to ensure the process tree is cleaned up when kill is invoked

node-pty/src/windowsTerminal.test.ts

Lines 104 to 126 in d738123

	it('should kill the process tree', function (done: Mocha.Done): void {
	this.timeout(20000);
	const term = new WindowsTerminal('cmd.exe', [], { useConpty, useConptyDll });
	// Start sub-processes
	term.write('powershell.exe\r');
	term.write('node.exe\r');
	console.log('start poll for tree size');
	pollForProcessTreeSize(term.pid, 3, 500, 5000).then(list => {
	assert.strictEqual(list[0].name.toLowerCase(), 'cmd.exe');
	assert.strictEqual(list[1].name.toLowerCase(), 'powershell.exe');
	assert.strictEqual(list[2].name.toLowerCase(), 'node.exe');
	term.kill();
	const desiredState: IProcessState = {};
	desiredState[list[0].pid] = false;
	desiredState[list[1].pid] = false;
	desiredState[list[2].pid] = false;
	term.on('exit', () => {
	pollForProcessState(desiredState, 1000, 5000).then(() => {
	done();
	});
	});
	});
	});

, the above issue indicates the test is incomplete.

Very likely we need some pending I/O work by the child processes to trigger the issue..

[deepak1556]

I will retag this as a bug since the default expectation of kill is to gracefully shutdown the processes, using taskkill is more of a manual cleanup that indicates a bug in the current logic.

[meganrogge]

Turns out it does appear that on Windows, this is already happening. Can repro this -- microsoft/vscode#250523 (comment), so will open a PR to specify this setting only applies to Linux/macOS and revert the change for those correspondingly.

[deepak1556]

Thats great to know, thanks!