Regression in #330 — the returned promise isn't actually merged with one that waits for the streams to be consumed

[novemberborn]

This line needs to invoke handlePromiseOnce:

execa/index.js

Line 156 in c9b4d09

return mergePromise(spawned, handlePromiseOnce);

[ehmicky]

Hi @novemberborn, thanks for this issue.

Could you please post an example of a code that breaks with the latest release?

For information, handlePromiseOnce() (and the underlying handlePromise()) are deferred until the user explicitly calls then(), catch() or await. This is because handlePromise() is reading stdout and stderr streams. If the user prefers to consume those streams themselves, handlePromise() should not be called.

Note that this behavior was not introduced by #330. That PR introduced the fact that handlePromise() is only called once.

[novemberborn]

Huh. In that case I suppose the problem is that I don't await promise until after I call promise.kill(). What you're saying is that I need to use then() first, kill(), and then drain the streams?

[novemberborn]

Will investigate further tomorrow.

[ehmicky]

The problem is that if we start consuming stdout and stderr right away, users won't able to directly consume those, if they prefer to do that instead of using promises. That's why we are deferring it.

On the other hand, please note that the child process is executed right away regardless of whether you call then(). So you should be able to call kill() without issues. Once a child process is destroyed, Node.js cleanup streams, I don't think you need to do anything there (please correct me if I'm wrong).

I am not sure I fully understand your initial problem though. If you're still experiencing some issues, could you please post an example showing your problem?

[novemberborn]

One problem may be, when the ExecaChildProcess promise is awaited, the code doesn't check if the stdout / stderr / all streams have already ended. So I need to use Promise.resolve(execaRetval) to set up the end listeners before consuming the streams.

[novemberborn]

Although I just changed my test to await the process directly and it passed again. Suffice to say there's some very confusing behavior here. Unfortunately I've got more pressing matters to deal with than upgrading execa. Will try again later. Sorry for the noise 😄

[ehmicky]

One problem may be, when the ExecaChildProcess promise is awaited, the code doesn't check if the stdout / stderr / all streams have already ended.

Yes at the moment, if you await ExecaChildProcess after the child process has ended, streams might have ended, and then stream consumption might fail. #322 is tracking this.

So I need to use Promise.resolve(execaRetval) to set up the end listeners before consuming the streams.

If you are consuming the streams directly (as opposed to using await), execa should not deal with them, including setting end listeners. If you are consuming the streams with await, Promise.resolve() should not be needed there?

Suffice to say there's some very confusing behavior here.

I don't understand what the core issue is. Feel free to provide with some broken test/code that's at the base of this problem, as this would help me understand it.