util: implement util.getSystemErrorName() by joyeecheung · Pull Request #18186 · nodejs/node

joyeecheung · 2018-01-16T20:27:45Z

Reimplement uv.errname() as internal/util.getErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

The next step would be to deprecate uv.errname() (since the binding seems to be used in the user land, e.g. execa), remove the C++ code and monkey patch the binding to internal/util.getErrorName() during bootstrapping, although I am not sure when during the bootstrapping process is the best time to do that.

Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name

const char* uv_err_name(int err)
Returns the error name for the given error code. Leaks a few bytes of memory when you call it with an unknown error code

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util, child_process, test

Reimplement uv.errname() as internal/util.getErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use.

joyeecheung · 2018-01-16T20:29:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12562/

addaleax

Looks good!

Only suggestion I’d have is maybe calling the method something like getSystemErrorName() just to be a bit more clear about what it does. Feel free to ignore that though :)

jasnell · 2018-01-16T20:41:22Z

lib/internal/util.js

+    return errmap.get(code)[0];
+  } else {
+    return `Unknown system error ${code}`;
+  }


Rather than the separate has() and get() calls, perhaps just

function getErrorName(code) { const entry = errmap.get(code); return entry ? entry[0] : `Unknown system error ${code}`; }

jasnell

Generally LGTM. I agree with @addaleax on the name suggestion.

jasnell · 2018-01-16T20:42:58Z

lib/internal/util.js

 }

+function getErrorName(code) {
+  if (errmap.has(code)) {


should code be type checked at all?

@jasnell Yeah, errname() does CHECK_LT(err, 0), we should probably check for the type and the ranges. Thanks for catching that.

vsemozhetbyt · 2018-01-16T21:06:22Z

doc/api/util.md

 [Customizing `util.inspect` colors]: #util_customizing_util_inspect_colors
 [Internationalization]: intl.html
 [WHATWG Encoding Standard]: https://encoding.spec.whatwg.org/
+[Common System Errors]: errors.html#common_system_errors


#common_system_errors -> #errors_common_system_errors )

vsemozhetbyt · 2018-01-16T21:09:28Z

doc/api/util.md

+```js
+fs.access('file/that/does/not/exist', (err) => {
+  const name = util.getErrorName(err.errno);
+  console.log(name);  // ENOENT


Maybe console.error(name)?

joyeecheung · 2018-01-16T21:56:55Z

@addaleax @jasnell @vsemozhetbyt Thanks for the reviews. A few updates:

getErrorName() -> getSystemErrorName()
Simplified the error map access logic
Moved the type-checking of _errnoException() into getSystemErrorName(), renamed the argument to err to be consistent with _errnoException()
Fixed the link in the documentation

PTAL

New CI: https://ci.nodejs.org/job/node-test-pull-request/12564/

jasnell · 2018-01-17T15:51:09Z

lib/internal/util.js

+  if (typeof err !== 'number') {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'err', 'number', err);
+  }
+  if (err >= 0 || !Number.isSafeInteger(err)) {


I believe that if (err >> 0 |== err) should work also

jasnell · 2018-01-17T15:51:49Z

Still LGTM :-)

BridgeAR · 2018-01-19T12:14:16Z

doc/api/util.md

+```js
+fs.access('file/that/does/not/exist', (err) => {
+  const name = util.getSystemErrorName(err.errno);
+  console.error(name);  // ENOENT


Suggestion: It might be good to also show the err.errno here to outline the difference.

@bridge The numeric code is platform-dependent so I am not sure if it's OK to just show the value of a particular platform or should I need to show all the possible values...

I guess you meant me ;-)

And true, I forgot about that. Hm, either a "e.g. on platform xyz" or just land it as is. What ever you think is best.

Reimplement uv.errname() as internal/util.getSystemErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use. PR-URL: #18186 Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

joyeecheung · 2018-01-20T01:12:55Z

Landed in 4af1bba, thanks!

joyeecheung · 2018-01-20T01:14:39Z

Note to backporters: this needs to land with c569727 which belongs to a semver-major PR. I will see if I can manually backport this.

Reimplement uv.errname() as internal/util.getSystemErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use. PR-URL: nodejs#18186 Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Reimplement uv.errname() as internal/util.getSystemErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use. Backport-PR-URL: #18916 PR-URL: #18186 Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * **libuv**: - Updated to libuv 1.19.2 (Colin Ihrig) [#18918](#18918) * **src**: - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [#14901](#14901) * **timers**: - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [#18139](#18139) * **util**: - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [#18186](#18186)

Notable Changes: * **libuv**: - Updated to libuv 1.19.2 (Colin Ihrig) [#18918](#18918) * **src**: - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [#14901](#14901) * **timers**: - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [#18139](#18139) * **util**: - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [#18186](#18186) PR-URL: #19040 Prepared-By: Anna Henningsen <anna@addaleax.net>

Reimplement uv.errname() as internal/util.getSystemErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use. PR-URL: nodejs#18186 Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * **libuv**: - Updated to libuv 1.19.2 (Colin Ihrig) [nodejs#18918](nodejs#18918) * **src**: - Add initial support for Node.js-specific post-mortem metadata (Matheus Marchini) [nodejs#14901](nodejs#14901) * **timers**: - The return value of `setImmediate()` now has `ref()` and `unref()` methods (Anatoli Papirovski) [nodejs#18139](nodejs#18139) * **util**: - It is now possible to get the name for a numerical platform-specific error code as a string (Joyee Cheung) [nodejs#18186](nodejs#18186) PR-URL: nodejs#19040 Prepared-By: Anna Henningsen <anna@addaleax.net>

Reimplement uv.errname() as internal/util.getSystemErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use. Backport-PR-URL: #19191 PR-URL: #18186 Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

util: implement util.getErrorName()

85a4230

Reimplement uv.errname() as internal/util.getErrorName() to avoid the memory leaks caused by unknown error codes and avoid calling into C++ for the error names. Also expose it as a public API for external use.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 16, 2018

addaleax approved these changes Jan 16, 2018

View reviewed changes

jasnell reviewed Jan 16, 2018

View reviewed changes

jasnell approved these changes Jan 16, 2018

View reviewed changes

jasnell reviewed Jan 16, 2018

View reviewed changes

vsemozhetbyt reviewed Jan 16, 2018

View reviewed changes

joyeecheung added 3 commits January 17, 2018 05:38

fixup! getErrorName -> getSystemErrorName

6ea1a42

fixup! simplify error map access

3b77a93

fixup! add typecheck, fix documentation

d696f73

joyeecheung changed the title ~~util: implement util.getErrorName()~~ util: implement util.getSystemErrorName() Jan 16, 2018

cjihrig approved these changes Jan 17, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018

jasnell reviewed Jan 17, 2018

View reviewed changes

lpinca approved these changes Jan 17, 2018

View reviewed changes

joyeecheung mentioned this pull request Jan 18, 2018

Investigate flaky sequential/test-async-wrap-getasyncid #18190

Closed

BridgeAR approved these changes Jan 19, 2018

View reviewed changes

joyeecheung closed this Jan 20, 2018

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2018

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

joyeecheung added backported-to-v9.x and removed backport-requested-v9.x labels Mar 6, 2018

joyeecheung mentioned this pull request Mar 7, 2018

[v8.x backport] backport 17338, 18186, 18358, 18546 #19191

Closed

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Uh oh!

Conversation

joyeecheung commented Jan 16, 2018

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Jan 16, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 20, 2018

Uh oh!

joyeecheung commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

joyeecheung commented Jan 16, 2018 •

edited

Loading

joyeecheung commented Jan 20, 2018 •

edited

Loading