Allow pty.kill to be awaited

[TheLazySquid]

Currently there isn't a good way to know when a pseudoterminal is fully killed, since there is some asynchronous logic on windows. This means that there isn't a good way to clean up any active pseudoterminals in electron before shutdown, as described in #733, because if the app is quit before the terminals are fully killed the process will hang. This PR allows pty.kill to be awaited, so the app can be shut down only after any pseudoterminals are fully killed, making a clean solution like this possible:

const { app } = require("electron");
const { spawn } = require("node-pty");

app.whenReady().then(async () => {
    let pty = spawn("cmd.exe");

    // process exits normally
    await pty.kill();
    app.quit();
});

[TheLazySquid]

@microsoft-github-policy-service agree

[lydell]

Shouldn’t the outer promise be rejected when the this._agent.kill() promise fails?

Suggested change

	return new Promise((res) => {
	this._deferNoArgs(() => {
	if (signal) {
	throw new Error('Signals not supported on windows.');
	}
	this._close();
	this._agent.kill().then(res);
	return new Promise((res, rej) => {
	this._deferNoArgs(() => {
	if (signal) {
	throw new Error('Signals not supported on windows.');
	}
	this._close();
	this._agent.kill().then(res, rej);

[lydell]

This may or may not be out of scope for this PR, but anyway.

As far as I can tell, the callbacks passed to this._deferNoArgs are called later, asynchronously. So it looks like the throw here cannot be caught. This is not a new issue in this PR. But with this PR, it’s less expected to throw errors since the function returns a Promise – I’d expect the promise to be rejected instead. We have the possibility to do that:

Suggested change

	return new Promise((res) => {
	this._deferNoArgs(() => {
	if (signal) {
	throw new Error('Signals not supported on windows.');
	}
	this._close();
	this._agent.kill().then(res);
	return new Promise((res, rej) => {
	this._deferNoArgs(() => {
	if (signal) {
	rej(new Error('Signals not supported on windows.'));
	} else {
	this._close();
	this._agent.kill().then(res, rej);
	}

However, I don’t know if this is wanted or not by end consumers. They can always check caughtError.message === 'Signals not supported on windows.' but 🤷

If this change is made, this line needs to be updated in node-pty.d.ts:

@throws Will throw when signal is used on Windows.

[TheLazySquid]

I think it makes more sense to reject the promise if signal is used, especially if the thrown error would sometimes not be caught. I'm not entirely sure how promise rejections should be described in jsdoc, but changing it to @throws Will reject when signal is used on Windows. seems to be fine.

[lydell]

Nice!

Note that your commit (unlike my suggestion above) changes behavior: Previously, this._close() and this._agent.kill() were not run if signal is truthy. In your commit, the promise is rejected and then those two methods are run anyway.

[TheLazySquid]

Whoops, should be fixed now.

[Tyriar]

Great change 👏

@rzhao271 looks like the SDL job is failing with a permissions problem.

[rzhao271]

I wouldn't be surprised if CI is failing because it is trying to run on a fork. #727, which splits the current pipeline into two and makes the CI pipeline use the unofficial template might help.

[Tyriar]

@rzhao271 can we get it to run on a PR branch that gets merged into main with an approve and run button? Alternatively, we could just have SDL blocking the release step and only run on main after the PR is merged.