expo/spawn-async

listen 'exit' event instead of 'close' as spawnSync does

konovalovsergey opened this issue · 2 comments

I think spawnSync and "spawn-async"(as Promise-based interface for spawn) should act in a similar way.
As shown by experiment(below) spawnSync returns when the 'exit' event is triggered.

The documentation currently says:

The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.

This experiment inspired by this discussion(stackoverflow). Pipe program with lots of output to another, kill first program:
With spawnSync

var cpPrev = require('child_process').spawn('bash', ['-c', 'mkfifo tmppipe && yes > tmppipe']);
setTimeout(function() {
	cpPrev.kill();
	console.log('cpPrev.kill()');
	require('child_process').spawnSync('bash', ['-c', 'rm tmppipe']);
}, 1000);

require('child_process').spawnSync('bash', ['-c', 'cat < tmppipe'], {timeout: 500});
console.log('spawnSync complete');

It outputs:

spawnSync complete
cpPrev.kill()

spawnSync completes before second command is killed.

Next with async spawn

var cpPrev = require('child_process').spawn('bash', ['-c', 'mkfifo tmppipe && yes > tmppipe']);

var cp = require('child_process').spawn('bash', ['-c', 'cat < tmppipe']);
cp.once('exit', console.log.bind(console, 'exited'));
cp.once('close', console.log.bind(console, 'closed'));

setTimeout(function() {
	cp.kill();
	console.log('cp.kill()');
	setTimeout(function() {
		cp.stdout.destroy();
		cpPrev.kill();
		console.log('cpPrev.kill()');
		require('child_process').spawnSync('bash', ['-c', 'rm tmppipe']);
	}, 500);
}, 500);

It outputs:

cp.kill()
exited null SIGTERM
cpPrev.kill()
closed null SIGTERM

'exit' event is trigged after cp is killed(as spawnSync). 'close' event is trigged after cp.stdout.destroy.

So, i think "spawn-async" should listen 'exit' event or add appropriate option.

Hi--

This might make sense. I think there was a reason that I made it listen to close instead of exit originally, but I can't remember what that was now.
If you submit a PR and it passes all the tests, we could potentially release a new major version that listens to exit instead.

ide commented

I spent a moderate amount of time looking into this and believe we need to listen to the "close" event to read stdout/stderr correctly. One of the tests that runs "echo hi" failed just once when I tried listening to "exit" instead of "close" and the test failure showed that spawnAsync didn't read stdout. I couldn't reproduce this test failure -- a bit worrisome since it'd be easy to accidentally introduce this regression -- but it makes sense since the subprocess could exit before stdout fully flushes and the parent reads it.

My understanding of the issue is that there are two different use cases: (1) to invoke a program and get its output and (2) to invoke a program and care only that it finishes. We've optimized for (1) today. If we were to add an option for (2), I think the correct thing to do is to skip collecting stdout/stderr since they'll return the correct data nearly all of the time but sometimes inexplicably be missing data and be extremely hard to debug.