Make the spawned process cancelable

[ammarbinfaisal1]

see if this is okay.
I was wondering whether there should we a check if oncancel property in options object passed by someone is actually a function. is this required?

Fixes #113

[sindresorhus]

You need to add tests and docs too.

[ammarbinfaisal1]

There is CancelError being thrown. In test.js and readme, I've passed an empty function () => {} to spawned.catch to ignore it. should I use a try catch block inside the main code to do this?

[sindresorhus]

No, you're missing the point of CancelError. Please read the p-cancelable readme and understand how it works.

[ehmicky]

I think it would be nice to add properties similar to how timedOut, failed and killed and currently handled:

• add result.canceled. Always defined (true or false) whether on success or failure. Exception: not defined with sync() since synchronous calls cannot be canceled.
• enhance result.message (getErrorPrefix() function) to add was canceled: ${reason}. : ${reason} should not be appended in it's default reason.

[ammarbinfaisal1]

I think it would be nice to add properties similar to how timedOut, failed and killed and currently handled:

add result.canceled. Always defined (true or false) whether on success or failure. Exception: not defined with sync() since synchronous calls cannot be canceled.
enhance result.message (getErrorPrefix() function) to add was canceled: ${reason}. : ${reason} should not be appended in it's default reason.

@ehmicky, I can't figure out how to do this. On cancelling p-cancelable itself throws CancelError and this is not notified in the handlePromise function - I mean in the if block which checks whether there is an error and and creates a proper execa error using makeError.

[ehmicky]

Something like this might work if I understand your problem correctly:

	const processDone = new PCancelable((resolve, reject, onCancel) => {
        let canceled = false;
		spawned.on('exit', (code, signal) => {
			cleanup();
			resolve({code, signal, canceled});
		});

		/* more code */

		onCancel(() => {
            canceled = true;
			spawned.kill();
		});
	});

[ammarbinfaisal1]

Something like this might work if I understand your problem correctly:

@ehmicky No, it won't. As soon as we cancel the promise p-cancelable itself throws the error as you can see here.

[ehmicky]

This might work if you put the canceled variable one level up:

let canceled = false;
const processDone = new PCancelable((resolve, reject, onCancel) => {
  /* more code */
  onCancel(() => {
    canceled = true;
    spawned.kill();
  });
});

const handlePromise = () => pFinally(Promise.all([
		processDone,
		getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
		getStream(spawned, 'stderr', {encoding, buffer, maxBuffer})
	]).then(results => { // eslint-disable-line promise/prefer-await-to-then
		const result = results[0];
		result.stdout = results[1];
		result.stderr = results[2];

		if (result.error || result.code !== 0 || result.signal !== null || canceled) {
			const error = makeError(result, {
				joinedCommand,
				parsed,
				timedOut,
                canceled
			});

			// TODO: missing some timeout logic for killed
			// https://github.com/nodejs/node/blob/master/lib/child_process.js#L203
			// error.killed = spawned.killed || killed;
			error.killed = error.killed || spawned.killed;

			if (!parsed.options.reject) {
				return error;
			}

			throw error;
		}

		return {
			stdout: handleOutput(parsed.options, result.stdout),
			stderr: handleOutput(parsed.options, result.stderr),
			code: 0,
			failed: false,
			killed: false,
			signal: null,
			cmd: joinedCommand,
			timedOut: false,
			canceled: false,
		};
	}), destroy);

Notice the || canceled condition: if the promise is rejected because of cancellation, exit code might still be 0 with no errors nor signals (in case child process killing happens on the next tick), but we want to make sure cancellation is taken into account.

[ehmicky]

Ok I just realized from my above comment that the processDone promise will be rejected with a CancelError, which won't work with the rest of the code which assumes otherwise.

However when we cancel the promise, the child process is killed. This will fire exit event which will resolve the promise. Also we do want to wait on the child process to be killed before returning the result.

So I suggest two possible solutions. First using onCancel.shouldReject = false.

Second not using p-cancelable and simply doing:

	const processDone = new Promise((resolve, reject) => {
		spawned.on('exit', (code, signal) => {
			cleanup();
			resolve({code, signal});
		});

		spawned.on('error', error => {
			cleanup();
			resolve({error});
		});

		if (spawned.stdin) {
			spawned.stdin.on('error', error => {
				cleanup();
				resolve({error});
			});
		}
	});

    let canceled = false;
    processDone.cancel = function() {
      if (canceled) { return; }

      canceled = true;
      spawned.kill();
    }

[ammarbinfaisal1]

@ehmicky I can't understand why is this happening but if I use noop, node forever instead of ls in the latest added test, it fails

  1 test failed

  test › calling cancel method makes result.cancel true


  Rejected promise returned by test. Reason:

  Error {
    cmd: 'node fixtures',
    code: 2,
    errno: 'ENOENT',
    failed: true,
    killed: false,
    path: 'node fixtures',
    signal: null,
    spawnargs: [],
    stderr: '',
    stdout: '',
    syscall: 'spawn node fixtures',
    timedOut: false,
    message: 'Command failed with exit code 2 (ENOENT): spawn node fixtures ENOENT',
  }

[ehmicky]

Could you post the code of that test?

From intuition it seems like you should be doing execa('noop') or execa('forever'). The fixtures directory is automatically added in the PATH.

[ammarbinfaisal1]

this is the failing test

test('calling cancel method makes result.cancel true', async t => {
	const spawned = execa('ls');
	spawned.cancel();
	const result = await spawned;
	t.true(result.canceled);
});

I think this depends on the time taken by the process like even with ls it failed one time on my PC and ran the next time

[ammarbinfaisal1]

I think using p-cancelable and allowing CancelError to be thrown is the best think which can be done; probably @sindresorhus asked to use p-cancelable because of that and why add a canceled property in result when the person using this can just check for CancelError(i mean use catch) 🤔

[ehmicky]

You're showing a different test that the initial failure. The initial failure you mentioned was about Command failed with exit code 2 (ENOENT): spawn node fixtures ENOENT. The one you are now posting uses a different command and is a separate problem. Let's try not to fork too many different discussions, otherwise it's hard to keep track. Hopefully my previous comment might help solve the initial problem.

What you are trying to test also depends on how you will implement this feature. If there is a timing issue there, the issue is probably not the test but the implementation. Calling cancel() directly calls spawn.kill(), so the child process should be killed before being able to do anything.

About letting the CancelError exception propagate, the issues are:

1. while process.kill() has been called, the child process might not be called right away (synchronously) by Node.js. This means the processDone() promise might be rejected (and execa() might end) while the child process is still being killed, which is not good.
2. because of 1), the result will be missing the termination signal and the code
3. it adds a new value to which processDone() can resolve, which leads to more polymorphic code, i.e. more complexity.

Either of the two solutions above would solve those issues (or any third solution you might think of).

[ammarbinfaisal1]

sorry for troubling you so much over this PR.

[ehmicky]

You are helping us add a new feature, there's nothing to be sorry about!

Code review is part of the process and I enjoy it, so no worries.

Hopefully this is a good experience for you as well, in case you wanted to contribute again after this PR :)

I think the PR looks good now! Only two things missing:

• add documentation about the cancel() method in the API section of the README so users know how to use it.
• final code review by @sindresorhus

[ehmicky]

@sindresorhus the PR looks good to me, but awaiting on your own code review before merging.

[sindresorhus]

I would prefer to name it isCanceled. I realize it's inconsistent with killed, but I want to rename that one at some point too. Use the isCanceled naming for all the canceled variables.

[sindresorhus]

Use arrow function

[sindresorhus]

Single-quotes

[sindresorhus]

I think process would be a better name than spawned.

[ehmicky]

Would this confuse readers about the global variable named process? Child processes returned by spawn() and global process have different methods/properties (although some are shared).

Node API doc calls it subprocess: https://nodejs.org/api/child_process.html#child_process_options_detached

[sindresorhus]

Ah yeah, subprocess is better.

[sindresorhus]

It's not clear enough what spawned is. And it's weird to place the docs here. I think it would be better to document it in the execa() section.

[sindresorhus]

Suggested change

	const spawned = execa('noop');
	const subprocess = execa('noop');

[sindresorhus]

Don't do unrelated changes

[ammarbinfaisal1]

I might have done this while fixing some merge conflict; that's the only time I touched those two variables exitCode and exitCodeName

[sindresorhus]

Thank you for contributing, @ammarbinfaisal :)