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

The all stream and stderr: 'inherit' #344

Closed
novemberborn opened this issue Jul 2, 2019 · 10 comments · Fixed by #353
Closed

The all stream and stderr: 'inherit' #344

novemberborn opened this issue Jul 2, 2019 · 10 comments · Fixed by #353

Comments

@novemberborn
Copy link

I'm using execa to launch a child process that logs JSON to stdout. Each of these lines are parsed and consumed.

I've set buffer to false. I'm inheriting stderr since I do not need to parse it.

I now need to fully consume both stdout, which is fair enough, and all, even though it only contains the data from stdout which I've already consumed.

Is there a way of opting out of using all?

@novemberborn novemberborn changed the title The all stream and `stderr: 'inherit' The all stream and stderr: 'inherit' Jul 2, 2019
@ehmicky
Copy link
Collaborator

ehmicky commented Jul 2, 2019

Thanks for reaching out @novemberborn!

Would this work for you if you consumed stdout and not all? Is your problem that the all stream might take up memory under the hood?

Side question: you need to inherit stderr, using ignore is not possible, correct? I'm asking because ignore would be more efficient.

Note: if by any chance you do not call result.then() or await result, you do need to use buffer: false since buffering is never used then.

@novemberborn
Copy link
Author

Side question: you need to inherit stderr, using ignore is not possible, correct? I'm asking because ignore would be more efficient.

I'm using execa inside an AVA test process to launch the process I'm testing against. Its log output goes over stdout but inheriting stderr means I can still use console.error() inside the process for debugging purposes.

Would this work for you if you consumed stdout and not all? Is your problem that the all stream might take up memory under the hood?

I don't care about memory. I think the solution may be to consume all instead of stdout, since it reads from stdout anyway. The stream promises wait for both streams to end, and all won't end if you don't read it…

Indeed I have another test which asserts against stdout and stderr. Awaiting the ExecaChildProcess does nothing unless I also fully consume the combined all stream.

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 3, 2019

Thanks for explaining this. Could you please provide with a code example so I can better understand your problem?

Indeed I have another test which asserts against stdout and stderr. Awaiting the ExecaChildProcess does nothing unless I also fully consume the combined all stream.

Could you provide with a code example reproducing this? This would be a bug.

@tiagonapoli
Copy link
Contributor

I think a workaround is to call .all.resume():

const proc = execa("echo", ["hello"], {
  buffer: false,
  stderr: "inherit"
});
proc.stdout.pipe(process.stdout);
proc.all.resume();

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 13, 2019

I am not fully understanding what the original problem of this issue nor the problem this workaround is fixing: @tiagonapoli could you please describe it?

For example taking your example, if I remove the proc.all.resume() line, then run the file, hello gets correctly printed on stdout.

@tiagonapoli
Copy link
Contributor

This snippet shows the problem I was experiencing (which was the issue I #350):

const execa = require("execa");

const go = async () => {
  try {
    const proc = execa("echo", ["hello"], {
      buffer: false,
      stderr: "inherit"
    });
    proc.stdout.pipe(process.stdout);
    // proc.all.resume();

    const procRes = await proc;
    console.log(procRes);
  } catch (err) {
    console.log("CAUGHT ERROR", err);
  }
  console.log("============= DONE ================");
};

go();

The output is correctly piped to process.stdout, but the proc promise doesn't resolves.

@ehmicky
Copy link
Collaborator

ehmicky commented Jul 13, 2019

Great, thanks! That is a problem indeed!

Your example can be reduced to simply:

const execa = require("execa");

(async () => {
  await execa("echo", { buffer: false });
  // This never gets printed
  console.log('Done');
})();

@tiagonapoli tiagonapoli mentioned this issue Jul 16, 2019
2 tasks
@ehmicky
Copy link
Collaborator

ehmicky commented Jul 16, 2019

PR at #353.

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 1, 2019

This should now be fixed. Also all is now an opt-in feature (before the all boolean option).

This change will be released in the next major release.

@novemberborn
Copy link
Author

Thanks for the update @ehmicky. Apologies for not having the time to investigate more deeply. Thanks for fixing this @tiagonapoli!

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 a pull request may close this issue.

3 participants