execa: Unfinished execa promise when path is not found and buffer=false

When running:

const proc = await execa("echo hello", [], {
  buffer: false
});

This promise never resolves or rejects.

I investigated the source and found the problem on: https://github.com/sindresorhus/execa/blob/071a8154f882d13116a6a91d8691ea150de31753/lib/stream.js#L60-L67

And fixed using:

if (!buffer) {
    // TODO: Use `ret = util.promisify(stream.finished)(stream);` when targeting Node.js 10
    return new Promise((resolve, reject) => {
        stream
            .once('end', resolve)
            .once('error', reject)
            .once('close', resolve)
    });
}

Adding .once('close', resolve) solved the problem, since when the file is not found, it seems child_process.spawn just closes the streams but not writes an end (?).

The solution proposed by the TODO comment works too, but I had a doubt about the targeting Node.js 10 part: The project is dropping compatibility with previous versions?

Another problem about the buffer option that I noticed is in the documentation:

buffer

Type: boolean
Default: true

Buffer the output from the spawned process. When buffering is disabled you must consume the output of the stdout and stderr streams because the promise will not be resolved/rejected until they have completed.

Consuming the stdout and stderr is not enough, the all stream has to be consumed too, or just resumed. (consuming only the all stream works too)

I would be glad to open a PR to fix these issues

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 29 (14 by maintainers)

Most upvoted comments

The tests worked on my machine. Did some tests for the bugs we found and everything seemed ok

tiagonapoli on Jul 15, 2019

I’ve done some experiments and the following seems to work:

add an all boolean option defaulting to false
not waiting for streams completion anymore

What do you think?

ehmicky on Jul 15, 2019

Some tests. Does this behavior makes sense?

test('buffer: false > promise resolves', async t => {
	await t.notThrowsAsync(execa('echo', ['hello'], {buffer: false}));
});

test('buffer: false > promise does not resolve when output is big and is not read', async t => {
	const {timedOut} = await t.throwsAsync(execa('node', ['-e', 'console.log("a".repeat(3_000_000))'], {buffer: false, timeout: 1e3}));
	t.true(timedOut);
});

test('buffer: false > promise resolves when output is big and is read', async t => {
	const cp = execa('node', ['-e', 'console.log("a".repeat(3_000_000))'], {buffer: false});
	cp.stdout.resume();
	cp.stderr.resume();
	await t.notThrowsAsync(cp);
});

test('buffer: false > promise does not resolve when output is big and "all" is used but not read', async t => {
	const cp = execa('node', ['-e', 'console.log("a".repeat(3_000_000))'], {buffer: false, all: true, timeout: 1e3});
	cp.stdout.resume();
	cp.stderr.resume();
	const {timedOut} = await t.throwsAsync(cp);
	t.true(timedOut);
});

test('buffer: false > promise resolves when output is big and "all" is used and is read', async t => {
	const cp = execa('node', ['-e', 'console.log("a".repeat(3_000_000))'], {buffer: false, all: true});
	cp.stdout.resume();
	cp.stderr.resume();
	cp.all.resume();
	await t.notThrowsAsync(cp);
});

test('buffer: false > promise resolves when output is big but is not pipable', async t => {
	await t.notThrowsAsync(execa('node', ['-e', 'console.log("a".repeat(3_000_000))'], {buffer: false, stdout: 'ignore'}));
});

ehmicky on Jul 15, 2019

Ok so this is a summary of this issue. Please let me know if I got it correctly.

1) Big `stdout` or `stderr` can make the child process not exit

The child process does not emit the exit event (i.e. the execa() returned promise does not resolve) when the either stdout or stderr:

is large (more than 219214 bytes on my machine)
uses stdio: 'pipe' (the default value)
is not read

Using buffer: true (the default value) reads all streams, so this only happens when buffer is false.

2) `all` must be consumed for the child process to exit, when `stdout` or `stderr` is big

When the conditions above are met, all also need to be read, for the same reasons. However all is different from stdout and stderr:

it is setup by execa not by Node.js core. It’s a PassThrough that gets both stdout and stderr as piped input.
most users might not be want to consume that stream, as it might be redundant with consuming stdout or stderr

3) Waiting for streams completion when `buffer` is `false` is problematic

When buffer is false and stdout, stderr or all uses stdio: 'pipe' (the default value), we wait for the stream to complete.

However:

we are waiting for end and error events, but should also wait for the close and finish events.
the child process will only end once its streams have completed, so this is unnecessary altogether.

ehmicky on Jul 15, 2019

execa: Unfinished execa promise when path is not found and buffer=false

buffer

About this issue

Most upvoted comments

1) Big stdout or stderr can make the child process not exit

2) all must be consumed for the child process to exit, when stdout or stderr is big

3) Waiting for streams completion when buffer is false is problematic

1) Big `stdout` or `stderr` can make the child process not exit

2) `all` must be consumed for the child process to exit, when `stdout` or `stderr` is big

3) Waiting for streams completion when `buffer` is `false` is problematic