Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add all option #353

Merged
merged 19 commits into from
Jul 31, 2019
Merged

Conversation

tiagonapoli
Copy link
Contributor

@tiagonapoli tiagonapoli commented Jul 16, 2019

Description

Fixes #350
Fixes #344

Currently, when buffer is set to false, execa wait for the process to finish, stdout, stderr and all streams to finish. However, in order for the process to certainly finish, the user will have to consume or just resume the all stream, because if all is not consumed and the process is output intensive, all, which is a PassThrough stream, will block the process output pipeline, preventing the process to close. So all stream has to be consumed or resumed and stdout,stderr can be optionally consumed.

It does make more sense for the user, when buffer=false, to consume only stderr and stdout, optionally consuming all, but for this behavior, all should be opt-in (if all is present, the user will have to consume it, otherwise the process may not finish), which makes sense in general, not only when buffer=false: in case the user genuinely wants to use all stream, he/she will opt-in for it.

When buffer=false another change that makes more sense is just to wait for the process to finish and not wait for the output streams. The user will have to consume them anyway, otherwise the process may not finish, so why not mirror child_process's behavior in this case and remove this complexity from execa?

So this PR proposes to make all opt-in, which is a breaking change, and fixes bugs reported on #350 when buffer=false.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change required a documentation update

Faulty behavior this PR solves

These bugs were detailed in #350, but this is a summary of them and the code below is the reason to 1 and 2 (and the fact that execa waits for stdout, stderr and all stream promises to finish):

execa/lib/stream.js

Lines 55 to 74 in 071a815

const getStreamPromise = (stream, {encoding, buffer, maxBuffer}) => {
if (!stream) {
return;
}
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);
});
}
if (encoding) {
return getStream(stream, {encoding, maxBuffer});
}
return getStream.buffer(stream, {maxBuffer});
};

  1. A bug on getStreamPromise when buffer=false. Currently, waiting for the end of stream is faulty: The all stream emits a finish event, instead an end event, because all is a Duplex, and its Writable part is in action (stdout and stderr are being piped into it), not its Readable part. If all is not consumed, execa never finishes because its waiting an end or error event from all.
const execa = require("execa");
(async () => {
  const proc = execa("echo", ["hello"], { buffer: false });
  proc.stdout.resume();
  proc.stderr.resume();
  const procRes = await proc;
  // This never gets printed
  console.log("done", procRes);
})();
  1. Another bug similar to 1). This time is because the streams are destroyed on error. When a stream is destroyed it emits only a close event, so stdio, stderr and all stream promises never end.
const execa = require("execa");
(async () => {
  // Intended mistake. The promise should reject with ENOENT
  const proc = execa("echo hello", { buffer: false });
  proc.stdout.resume();
  proc.stderr.resume();
  try {
    const procRes = await proc;
  } catch(err) {
    // This never gets printed
    console.log(err)
  }
  
})();
  1. A complication on 1) and 2). The solution to the previous bugs was just fix getStreamPromise when buffer=false, meaning listen to the correct events for stream completion. The following bug forces the user to consume all, because otherwise the process will never finish, since all blocks the output pipeline. This is the cause to make all opt-in.
const execa = require("execa");
(async () => {
  // With big output the process doesnt exit until all is resumed (all blocks the pipeline)
  const proc = execa('node', ['-e', 'console.log("a".repeat(3000000))'], {buffer: false});
  proc.stdout.resume();
  proc.stderr.resume();
  // proc.all.resume(); 
  const procRes = await proc;
  //if all is not resumed, this never gets printed
  console.log("done")
})();

@ehmicky ehmicky changed the title Feature/all option Add all option Jul 16, 2019
@ehmicky
Copy link
Collaborator

ehmicky commented Jul 16, 2019

This looks good to me (@tiagonapoli and I both contributed to this PR).

Only one thing: could you add a test for the following? We have a fail fixture that might be used for this.

Another bug similar to 1). This time is because the streams are destroyed on error. When a stream is destroyed it emits only a close event, so stdio, stderr and all stream promises never end.

We then just need to wait for a code review from @sindresorhus.

@tiagonapoli
Copy link
Contributor Author

tiagonapoli commented Jul 16, 2019

Added on stream.js tests, would it be ok? 04a107b

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 16, 2019

Yes looks good!

@sindresorhus
Copy link
Owner

Is there really no way we can fix this without introducing an option? (Let’s think hard about that first) Execa already has way too many options.

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 17, 2019

The core problem is that the all feature relies on piping stdout and stderr to a new stream. If that stream is not consumed and stdout or stderr has lots of output, they will pause so that stream can catch up. This causes the child process not to exit.

Because it introduces that extra assumption for the user (all must be consumed), that feature should be an opt-in instead of an opt-out. Using interleaved stdout/stderr is a special use case anyway.

We went back and forth on this with @tiagonapoli but could not find an alternative.

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 17, 2019

Tagging @tomsotte as well if he wants to weigh in since he worked on the initial feature.

@tomsotte
Copy link
Contributor

I agree with @ehmicky that the all stream could be opt-in feature as it's a special use case.
But even if it is possible to find another solution, like hijacking some stream internal method, an hack is always an hack and it will probably haunt the project later on.

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 23, 2019

I agree with @tomsotte that the all stream could a little be described as a hack (somewhat). And that it currently creates some important issues, and might create more in the future, hereby it should be an opt-in.

@sindresorhus
Copy link
Owner

Can we make all lazy so that the PassThrough stream is only created if someone uses all? (Sorry if I’m missing something. I’m just trying to push for simplicity).

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 23, 2019

Unless I'm missing something, lazy instantiation is not possible because once data has been written to stdout or stderr, if all has not been created yet, the data will be lost and not pipeable anymore to all.

@tiagonapoli
Copy link
Contributor Author

Any new thoughts on this matter?

@sindresorhus sindresorhus merged commit 56b749d into sindresorhus:master Jul 31, 2019
@ehmicky
Copy link
Collaborator

ehmicky commented Aug 1, 2019

This requires a major release, but I would like for #351 to be resolved since this is a breaking change as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unfinished execa promise when path is not found and buffer=false The all stream and stderr: 'inherit'
4 participants