Fix mixing stream and promise consumption

[ehmicky]

Fixes #322

At the moment, we delay child process streams being consumed by get-stream until execaPromise.then() is called. However this does not work for the following reason:

• if the child process exits before execaPromise.then() is called, its streams will be closed when get-stream starts reading them.
• which means execaResult.stdout and execaResult.stderr will be empty.

This PR solves this problem by not delaying get-stream consumption.

The drawback (and breaking change) is that users would now need to pass the buffer: false option when not consuming execaPromise as a promise. Otherwise:

1. get-stream would buffer streams into memory
2. execaPromise.stdout|stderr.pipe() will only work if done in the same tick as execa()

The core issue is that we are consuming child process streams in two ways:
a. with get-stream (promise consumption)
b. we let users consume it themselves

Mixing those two creates several issues since streams can be piped to several streams at once, but not consumed twice.

Some refactoring can be done and some dependencies can be removed following this PR, but we can do this in a separate PR.

@tiagonapoli @sindresorhus do you have some feedback on this?

[sindresorhus]

It seems like a lot of our problems are caused by having execa() be both for promise and streams... Maybe we should rethink that?

[ehmicky]

I agree that mixing promises and streams means that child processes streams are being consumed by competing resources:

• the execa library (stream buffering when using promises)
• the execa users (stream direct consumption)

I think it would be better to not allow both at the same time. But it would be a major breaking change, not sure if that's a good idea.

[sindresorhus]

Even if we decide it's not worth the breaking change in the end, it would still be productive to discuss how it would like like to split them up.

[ehmicky]

Core Node.js actually split them up: spawn() returns a child process while execFile() returns the buffered stdout/stderr.

A possible approach would be for us to do the same:

• execa() would still return a promise, but not mixed with a child process anymore
• we add a separate method to return the child process but without the promise/buffering logic

What do you think?

[kevva]

What are the advantages of mixing them besides having a nicer syntax? I have a few modules where I do this as well, but I've yet to see any real use cases where get-stream doesn't do the job.

we add a separate method to return the child process but without the promise/buffering logic

I think having a separate .stream() method is good, both for avoiding the problems outlined in this PR, but also because it's explicit. It's also what got is doing.

[novemberborn]

This trips me up all the time:

• I use execa to launch my service as part of an AVA test suite, with buffer: false
• I inherit stderr, since it's used for debugging
• I pipe stdout through split2 to parse the JSON log lines, which I assert against
• I want to wait for the service to shut down cleanly

I need to call .then() on the ExecaChildProcess to correctly set up that promise. But if I don't do this immediately, somehow .all also exists which seems contrary to the documentation, so I need to consume that too. And I'm just super confused.

This seems to work, but I can't really put my finger on why:

const bootstrap = (name: string) => { // eslint-disable-line @typescript-eslint/promise-function-async
  const service = execa.node(`${name}.js`, {
    buffer: false,
    cwd: 'build/test/fixtures/bootstrapped-services',
    env: {
      HEALTHCHECK_PORT: '0',
      LOG_LEVEL: 'debug',
    },
    stderr: 'inherit',
  }) as any as ExecaChildProcess<NodeJS.ReadableStream>

  const promise = service.then(result => result)
  if (service.all !== undefined) service.all.on('data', () => {})

  return {
    ...service,
    then: promise.then.bind(promise),
    catch: promise.catch.bind(promise),
    finally: promise.finally.bind(promise),
  }
}

Apologies for dumping this here rather than in a new issue, but it seems relevant to the discussion.

[ehmicky]

@sindresorhus what do you think?

[ehmicky]

After giving this PR some thoughts, I think this should be closed.

The original problem is probably not one. If a process output is consumed (here the user with childProcess.pipe()), that output should be discarded after consumption. If that output is being consumed by a second consumer (here execa with get-stream), it makes sense that the previous output is gone. This PR defaults to eager consumption instead of lazy consumption, which is probably not a good thing.

When it comes to the second problem raised in this PR (mixing different consumption patterns), I do think it would be better if those were separate. However I think the benefits of having them separate is not strong enough to justify such a breaking change. I also think this is a separate issue from this PR.

I am going to close this PR, but feel free to re-open it (if you disagree) and to comment. Thanks!

[spalger]

Now that this is resolved can we expect v3 soonish? (referencing #353 (comment))

[ehmicky]

We can make a major release but maybe we should first review/merge #375.

@sindresorhus what do you think?

[ffMathy]

Now that #375 has been merged, I guess we can make that major release?

[ehmicky]

Done at https://github.com/sindresorhus/execa/releases/tag/v3.0.0

[ffMathy]

Awesome work @ehmicky <3

[wesleytodd]

I think this should be revisited. The fact that there is currently no way to return the result of a call to execa from an async function and then access the stream is a pretty tough situation. As you can see in my example in #409, The function does some other things and then I would like to return the stream. Currently I have a hack like this:

async function execThing (args, asStream) {
  const cmd = await getCmd()
  const proc = execa(cmd, args)
  if (asStream) {
    delete proc.then
  }
  return proc
}

I think you will probably agree this is unfortunate. I think there are two reasonable api's which would help in my use case:

execa(cmd, args, { stream: true })
execa(cmd, args).stream()

Based on this conversation @sindresorhus said we should explore this, but I don't really see that exploration or conversation here. I just see the decision by @ehmicky to hold off since it is a rather significant breaking change.

I think that both approaches listed could be done in a non-breaking way. Make the default behavior still mix the promise and child process, but when stream: true or .stream() is called before .then it can just abort that behavior and strip out the promise parts. Thoughts?

[ehmicky]

I would be ok with this suggestion. @sindresorhus What are your thoughts?

[sindresorhus]

Sounds good. I would do execa(cmd, args).stream() as it's usually best to not let options totally change the return type.

[ehmicky]

Opened an issue at #414 to track this.