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

promisify child_process.exec always rejects #19494

Closed
njtrettel opened this issue Mar 20, 2018 · 10 comments
Closed

promisify child_process.exec always rejects #19494

njtrettel opened this issue Mar 20, 2018 · 10 comments
Labels
child_process Issues and PRs related to the child_process subsystem. promises Issues and PRs related to ECMAScript promises.

Comments

@njtrettel
Copy link

njtrettel commented Mar 20, 2018

  • Version:
    v8.9.4
  • Platform:
    Ubuntu 16.04

I'm having trouble promisifying child_process.exec. When I run with error or without, the promise is rejected. I've followed the docs and wrote this:

const { promisify } = require('util');
const execCallback = require('child_process').exec;
const exec = promisify(execCallback);

const fooBar = () => {
  const dir = `engine/Users/${user}`;
  const fileOne = `${process.cwd()}/${dir}/myFileOne.txt`;
  const fileTwo = `${process.cwd()}/${dir}/myFileTwo.txt`;
  const command = `diff --unchanged-group-format="" ${fileOne} ${fileTwo}`;

  return run(command)
    .then((stdout, stderr) => {
      console.log('ran diff');
      return stdout;
    })
    .catch(error => console.log('got error running diff'));
};

fooBar goes through the catch block with this error:

{
  killed: false,
  code: 1,
  signal: null,
  cmd: /*the correct diff cmd*/,
  stdout: /*the correct diff stdout*/,
  stderr: ''
}

An actual error (or at least a 'no such file') gives me code: 2, stdout: '', stderr: no such file error string

@jiripospisil
Copy link

I don't think this is an actual bug. diff returns 1 if files differ, 0 if they're the same.

∴ diff <(echo testing) <(echo testing)
∴ echo $?
0
∴ diff <(echo testing) <(echo testing2)
1c1
< testing
---
> testing2
∴ echo $?
1

The exec function considers any exit code other than 0 to be an error. Try running the code with the files being the same.

@vsemozhetbyt vsemozhetbyt added child_process Issues and PRs related to the child_process subsystem. promises Issues and PRs related to ECMAScript promises. labels Mar 20, 2018
@benjamingr
Copy link
Member

Yes, this is by design, this is also how the callback version works so it doesn't actually have to do with promises.

Try

  const command = `diff --unchanged-group-format="" ${fileOne} ${fileTwo} || true`;

@benjamingr
Copy link
Member

Maybe we should add a clarification to the docs: "If the process exits with a non-zero status code the promise is rejected"

@TomCoded
Copy link
Contributor

The docs for child_process exec already show that "Any exit code other than 0 is considered to be an error." https://github.com/nodejs/node/blame/master/doc/api/child_process.md#L193

@benjamingr
Copy link
Member

@TomCoded correct, I was suggesting adding that clarification to the util.promisify section:

If this method is invoked as its [util.promisify()][]ed version, it returns
a Promise for an object with stdout and stderr properties. In case of an
error (including a non-zero status code), a rejected promise is returned, with the same error object given in the
callback, but with an additional two properties stdout and stderr.

Or something of the sort

@njtrettel
Copy link
Author

@TomCoded thanks, definitely missed that. I just assumed it would be on a fatal error of some sort, and would have a stderr.

Checking the exit code will be fine.

@TomCoded
Copy link
Contributor

@benjamingr Oh, yes, something in the promisify section might be helpful. Perhaps (including any error resulting in an exit code other than 0), to keep the language consistent and to disambiguate from "an error including a non-zero exit code"

@benjamingr
Copy link
Member

@TomCoded would you be interested in submitting such a PR? (If you're unsure how to get started, let me know and we can walk through it on IRC/IM together, I also recommend checking out CONTRIBUTING.md)

@TomCoded
Copy link
Contributor

Will do.

TomCoded added a commit to TomCoded/node that referenced this issue Mar 22, 2018
Promisify section of child_process.exec doc changed to make clear
that non-zero exit codes will result in promise rejection.

fixes: nodejs#19494
@TomCoded
Copy link
Contributor

@benjamingr Submitted PR. (Thank you for the offer. I believe it worked, but I am still figuring out the system. Please have no qualms about suggestions/criticism.)

Cheers,
Tom

TomCoded added a commit to TomCoded/node that referenced this issue Mar 22, 2018
Promisify section of child_process.execFile doc changed to make
clear that non-zero exit codes will result in promise rejection.

fixes: nodejs#19494
targos pushed a commit that referenced this issue Mar 24, 2018
Promisify sections of `child_process.exec()`
and `child_process.execFile()` changed to make clear
that non-zero exit codes will result in promise rejection.

PR-URL: #19541
Fixes: #19494
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants