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

[errors] Add built in ErrnoError #9125

Closed
NoHomey opened this issue Oct 17, 2016 · 3 comments
Closed

[errors] Add built in ErrnoError #9125

NoHomey opened this issue Oct 17, 2016 · 3 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. feature request Issues that request new features to be added to Node.js.

Comments

@NoHomey
Copy link

NoHomey commented Oct 17, 2016

  • Version: 6.8.1
  • Platform: Linux nohomey 4.4.0-43-generic ee.getMaxListeners() #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: util, spawn, dns .... (all modules that throw the so known System Error)

System Errors are documented in the API Documentation and they are thrown all over the code, such examples are: util, spawn and dns. So I would like to propose to add built in ErrnoError class that can be used both by addon developers, node and even some user JavaScript code and to deprecate the current System Errors which are just augmented Errors. ErrnoError constructor may accept two arguments errno of type int and syscall of type string and it should generate proper error message like system call X failed with Y errno code, {human readable meaning}. ErrnoError may also be separated in it's own header/source where errno codes are also mapped and as well available to addon writes then exposed to JavaScript, which is currently done in node_constants.
And finally why I want such change to be made: to remove redundancy, as a node addon developer some times I need to define a macro that throws System Errors and there is no built in one so I have to either re-use the implementation from my previous project or created new one that slightly differs from the previous one - there is no standart way how Error instances are augmented, this redundancy can even be found in node's code itself: again in util, spawn and dns but is in JavaScript code not C++ ...

@mscdex mscdex added feature request Issues that request new features to be added to Node.js. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 17, 2016
@bnoordhuis
Copy link
Member

What you are asking for exists and has for a long time: node::ErrnoException() from src/node.h. I'm going to go ahead and close this.

@NoHomey
Copy link
Author

NoHomey commented Oct 23, 2016

@bnoordhuis I've checked node::ErrnoException and found that it slightly differs from the documented System Error in that, it sets errno to the passed errorno instead of it's string representation, like code to estring. In node's source ThrowErrnoException is used to throw ErrnoExceptions and I can't find a single place in node's source (C++) where the thrown ErrnoException is caught and it's errno is properly set to the string representation. However I do find places where this is done from the JavaScript code: (going to repeat my self with the examples from above: util, spawn and dns).

Is setting errno to the actual errno the correct behavior?

If not I do found this to be big redundancy since errno could be set directly to the same as code from node::ErrnoException and I would like to make a pull request to fix this issue in such case.

@bnoordhuis
Copy link
Member

Is setting errno to the actual errno the correct behavior?

Yes. If anything, the inconsistencies are in lib/ because we don't always set .code and .errno to a string and a number respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

3 participants