Impossible to trap errors from child process

[heyimalex]

Howdy! This is an explanation for #56

For various reasons, cmd isn't in always in the system path on windows. This makes calls to spawn fail with an ENOENT.

This wouldn't be an issue on its own, except that open gives no way to trap that error. Because the open function is async, by the time the promise resolves and we have access to a ChildProcess that we can add an error event listener to, the error event has already fired and bubbled up to the top level, crashing the program with an ENOENT.

Repro for windows:

• change "cmd" in index.js to some non-existent binary like "abcmd"
• create a test file that attempts to catch the error:

  open('http://github.com')
    .then(proc => {
      proc.on('error', () => {}); // Trap child process error.
    })
    .catch(() => {}); // Prevent `unhandledRejection` error, just in case.

• error is still thrown

Full repro available here.

git clone https://github.com/heyimalex/open.git -b issue_144_repro open_issue_144
cd open_issue_144
npm install
node repro.js

[atlefren]

Some investigation:

Given an environment with no cmd (as described above)

edit index.js (https://github.com/sindresorhus/open/blob/master/index.js) line 131 and onwards:

	subprocess.unref();

	subprocess.on('error', function( err) {
		console.log('error from subprocess')
	});
	console.log("return subprocess")
	return subprocess;

Then import index.js in another file and run:

var options = { app: browser, wait: false };
open(url, options).then(process => {
      console.log("got process")
      process.on('error', function( err) {
        console.log('got error')
      })
      
    }).catch((e) => {
      console.log(e)
    });

This will print:

return subprocess
error from subprocess
got process

This means that the subprocess error is emitted before the caller gets the process. This means that we will not be able to listen for the error event in the caller. This also means that we cannot throw an error in the event callback in open, as this will fire too late.

One possibility is to force a wait, this seems to work. The problem here is that when things work out, we will be waiting for the process to exit for a loooong time..

One (hacky?) solution is to always "wait", but when not wait is set, use a setTimeout to resolve if things does not go wrong after a (short) while, someting like this seems to wotk

return new Promise((resolve, reject) => {
		subprocess.once('error', reject);

		subprocess.once('close', exitCode => {
			if (exitCode > 0) {
				reject(new Error(`Exited with code ${exitCode}`));
				return;
			}

			if (options.wait) {
				resolve(subprocess);
			}

		});
		if (!options.wait) {
			setTimeout(() => {	resolve(subprocess);}, 100);
		}
	});

[heyimalex]

Some other solutions:

• Look up the executable in the path manually before calling spawn. This is actually how it's handled in go's stdlib os/exec package, so there's some precedence here. However, doesn't prevent the broader issue of attaching error listeners to a process returned from an async function.
• Allow a callback option that's immediately called with the created ChildProcess so that we can synchronously attach listeners. This is kind of ugly but gets the job done.
• Provide some argument to ignore errors all together. I'd imagine "try your best to open, but don't worry if you can't" is a pretty common use case.

[atlefren]

Option 1 is rather straight-forward (I think):

	if (process.platform === 'linux' && isWsl) {
		const path = childProcess.spawnSync('which', ['cmd.exe']).output.toString().split(',')[1];
		if (path === '') {
			throw new Error('Cannot find cmd.exe');
		}
	}