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

.errno field of errors from child_process.execSync is in string form instead of numeric. #12819

Closed
daurnimator opened this issue May 3, 2017 · 13 comments
Labels
child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core.

Comments

@daurnimator
Copy link

daurnimator commented May 3, 2017

  • Version: v7.9.0
  • Platform: Linux daurn-m73 4.10.11-1-ARCH #1 SMP PREEMPT Tue Apr 18 08:39:42 CEST 2017 x86_64 GNU/Linux
  • Subsystem: child_process

The .errno field of errors from child_process.execSync is in string form instead of numeric.

$ node -e 'try{require("child_process").execSync("foo", {shell: "doesnt_exit"})}catch(e){console.log(e.errno)}'
ENOENT

In this example I expect the numeric value -2 (as it is (correctly) is for other operations such as open:
try{require("fs").openSync("doesnt_exist", "r")}catch(e){console.log(e.errno)}).

@vsemozhetbyt vsemozhetbyt added child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels May 4, 2017
@daurnimator
Copy link
Author

Possibly related to #6665

@vsemozhetbyt
Copy link
Contributor

cc @bnoordhuis, @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented May 12, 2017

This comes from the use of util._errnoException(). Maybe that function shouldn't attach the same value for both code and errno, but it looks like it has been that way for 4 years now.

@jasnell
Copy link
Member

jasnell commented May 12, 2017

FWIW, this will be changing in an upcoming semver-major as part of the conversion over to internal/errors

@daurnimator
Copy link
Author

FWIW, this will be changing in an upcoming semver-major as part of the conversion over to internal/errors

great. is there milestone or related issue/PR for that?

@joyeecheung
Copy link
Member

@daurnimator Tracking issue for the ongoing internal/errors work is in #11273

(The issue in the OP is still present in v8.0.0..)

@daurnimator
Copy link
Author

daurnimator commented Feb 13, 2018

#11273 is now closed, but the issue remains. (tested with 9.5.0)

@bnoordhuis
Copy link
Member

Changing the type is a semver-major change so node 10 would be soonest you'd see that change, and assuming it doesn't cause breakage in the ecosystem.

@joyeecheung
Copy link
Member

I left a few TODOs in internal/errors.js

// TODO(joyeecheung): errno is supposed to err, like in uvException

// TODO(joyeecheung): errno is supposed to err, like in uvException

// TODO(joyeecheung): errno is supposed to be err, like in uvException

I would not oppose to try to have them fixed before v10 and see if there are any serious breakage...@jasnell what do you think?

@daurnimator
Copy link
Author

Could this be added to the v10 milestone? https://github.com/nodejs/node/milestone/26

@daurnimator
Copy link
Author

Issue remains in v11.11.0

@gireeshpunathil
Copy link
Member

@daurnimator - this is fixed through #28140 and is available is v13.0.0. Can you pls check at your end and revert?

#node -e 'try{require("fs").openSync("doesnt_exist", "r")}catch(e){console.log(e.errno)}'
-2
#node -v
v13.0.1
#

@daurnimator
Copy link
Author

@gireeshpunathil thanks for letting me know!
Fix verified.

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. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

No branches or pull requests

7 participants