lib: correct error.errno to always be numeric #28140

joyeecheung · 2019-06-09T14:51:10Z

The first commit is taken from #28139

lib: correct error.errno to always be numeric

Historically error.errno of system errors thrown by Node.js
can sometimes be the same as err.code, which are string
representations of the error numbers. This is useless and incorrect,
and results in an information loss for users since then they
will have to resort to something like
process.binding('uv')[UV_${errno}] to get to the numeric
error codes.

This patch corrects this behavior by always setting error.errno
to be negative numbers. For fabricated errors like ENOTFOUND,
error.errno is now undefined since there is no numeric equivalent
for them anyway. For c-ares errors, error.errno is now undefined
because the numeric representations (negated) can be in conflict
with libuv error codes - this is fine since numeric codes was
not available for c-ares errors anyway.

Users can use the public API util.getSystemErrorName(errno)
to retrieve string codes for these numbers.

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

nodejs-github-bot · 2019-06-09T14:51:12Z

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23864/

nodejs-github-bot · 2019-06-09T14:53:24Z

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

addaleax · 2019-06-09T18:30:43Z

Looks like the Windows failures are genuine … I’d like to run CITGM here, but I’m not sure if that makes sense before tests are passing?

nodejs-github-bot · 2019-06-09T20:00:29Z

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

nodejs-github-bot · 2019-06-10T10:21:07Z

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

nodejs-github-bot · 2019-06-10T15:24:22Z

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

doc/api/errors.md

Trott

LGTM if CITGM doesn't reveal anything too unexpected

Historically `error.errno` of system errors thrown by Node.js can sometimes be the same as `err.code`, which are string representations of the error numbers. This is useless and incorrect, and results in an information loss for users since then they will have to resort to something like `process.binding('uv'[`UV_${errno}`])` to get to the numeric error codes. This patch corrects this behavior by always setting `error.errno` to be negative numbers. For fabricated errors like `ENOTFOUND`, `error.errno` is now undefined since there is no numeric equivalent for them anyway. For c-ares errors, `error.errno` is now undefined because the numeric representations (negated) can be in conflict with libuv error codes - this is fine since numeric codes was not available for c-ares errors anyway. Users can use the public API `util.getSystemErrorName(errno)` to retrieve string codes for these numbers.

joyeecheung · 2019-06-13T16:43:32Z

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

joyeecheung · 2019-06-17T02:21:49Z

Landed in 1432065

Historically `error.errno` of system errors thrown by Node.js can sometimes be the same as `err.code`, which are string representations of the error numbers. This is useless and incorrect, and results in an information loss for users since then they will have to resort to something like `process.binding('uv'[`UV_${errno}`])` to get to the numeric error codes. This patch corrects this behavior by always setting `error.errno` to be negative numbers. For fabricated errors like `ENOTFOUND`, `error.errno` is now undefined since there is no numeric equivalent for them anyway. For c-ares errors, `error.errno` is now undefined because the numeric representations (negated) can be in conflict with libuv error codes - this is fine since numeric codes was not available for c-ares errors anyway. Users can use the public API `util.getSystemErrorName(errno)` to retrieve string codes for these numbers. PR-URL: #28140 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Trott · 2019-10-02T13:28:08Z

Looks like this landed without a CITGM run. Unfortunately, this change breaks execa (which is a dependency in csv-parser in CITGM.)

execa gets over 20 million downloads a week.

Trott · 2019-10-02T13:40:22Z

Looks like this landed without a CITGM run. Unfortunately, this change breaks execa (which is a dependency in csv-parser in CITGM.)

execa gets over 20 million downloads a week.

PR to make execa compatible with this change: sindresorhus/execa#370

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

This does not cover every possible permutation between v8 and v14. One difference was traced to nodejs/node#28140 in v13.

nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 9, 2019

joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 9, 2019

jasnell approved these changes Jun 10, 2019

View reviewed changes

cjihrig approved these changes Jun 10, 2019

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

Trott approved these changes Jun 12, 2019

View reviewed changes

joyeecheung added 2 commits June 12, 2019 17:10

fixup! lib: correct error.errno to always be numeric

6612645

joyeecheung force-pushed the errno branch from 30dd525 to 6612645 Compare June 12, 2019 15:16

trivikr approved these changes Jun 12, 2019

View reviewed changes

joyeecheung closed this Jun 17, 2019

BridgeAR added the dont-land-on-v12.x label Jun 17, 2019

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

gireeshpunathil mentioned this pull request Dec 30, 2019

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

Closed

richardlau mentioned this pull request Jan 21, 2020

-71 instead of EPROTO error #31438

Closed

workeitel mentioned this pull request Aug 10, 2020

UnknownEndpoint exception not thrown with Node14 aws/aws-sdk-js#3393

Closed

tpatel mentioned this pull request Feb 24, 2021

💡 Changed the lock_key column constraint to primary key TryGhost/knex-migrator#243

Merged

jturner-22 added a commit to Opto22/node-red-contrib-groov-io that referenced this pull request Nov 9, 2022

Get unit tests passing against Node.js 14.

7131146

This does not cover every possible permutation between v8 and v14. One difference was traced to nodejs/node#28140 in v13.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: correct error.errno to always be numeric #28140

lib: correct error.errno to always be numeric #28140

joyeecheung commented Jun 9, 2019 •

edited

nodejs-github-bot commented Jun 9, 2019 •

edited

nodejs-github-bot commented Jun 9, 2019

addaleax commented Jun 9, 2019

nodejs-github-bot commented Jun 9, 2019

nodejs-github-bot commented Jun 10, 2019

nodejs-github-bot commented Jun 10, 2019

Trott left a comment

joyeecheung commented Jun 13, 2019

joyeecheung commented Jun 17, 2019

Trott commented Oct 2, 2019

Trott commented Oct 2, 2019

lib: correct error.errno to always be numeric #28140

lib: correct error.errno to always be numeric #28140

Conversation

joyeecheung commented Jun 9, 2019 • edited