src: add error codes to errors thrown in C++ #27700

yanivfriedensohn · 2019-05-14T20:26:19Z

This PR follows #20121 and adds error codes to a few errors thrown in C++.

Moreover, InlineDecoder::Decode has been refactored to avoid throwing an exception related to StringBytes::IsValidString inside the Decode method.

Checklist

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

sam-github · 2019-05-14T22:39:51Z

Adding .code looks good to me, but I'm not familar enough with these APIs to see if this is semver-major or not, or if this is the correct way to handle string encoding.

nodejs-github-bot · 2019-05-20T03:30:36Z

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

src/node_buffer.cc

nodejs-github-bot · 2019-05-20T18:05:00Z

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

test/parallel/test-crypto.js

src/node_crypto.cc

tniessen · 2019-05-20T20:14:41Z

src/node_crypto.cc

+    if (!StringBytes::IsValidString(args[0].As<String>(), enc)) {
+      return THROW_ERR_INVALID_ARG_VALUE(env, "Invalid encoding for data");
+    }
+


This seems like an anti-pattern to me. You are effectively moving error handling from Decode() to different places, however, ParseEncoding is now called in multiple different places, too.

We prefer throwing the JS exception in the binding instead of in nested C++ functions.

Ideally this should be done in JS land, though.

Moving error handling out is okay as long as the callee still properly checks parameters. Your comment below would mitigate the problem :)

I moved the error handling to the binding.

src/string_bytes.h

lib/buffer.js

src/string_bytes.cc

nodejs-github-bot · 2019-05-27T03:41:05Z

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

lib/internal/util.js

nodejs-github-bot · 2019-05-28T21:53:37Z

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

yanivfriedensohn · 2019-06-04T19:27:55Z

@tniessen @joyeecheung pinging to check if the open conversations can be resolved.

joyeecheung

LGTM with nits

src/node_crypto.cc

src/string_bytes.cc

yanivfriedensohn · 2019-06-14T10:28:14Z

@tniessen @addaleax pinging to check if this PR can be approved.

nodejs-github-bot · 2019-06-27T19:06:48Z

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

nodejs-github-bot · 2019-06-27T19:08:02Z

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

joyeecheung · 2019-06-27T19:08:39Z

Jenkins is unable to rebase this PR onto master. @yanivfriedensohn can you rebase locally and push to the PR branch?

yanivfriedensohn · 2019-06-28T09:05:12Z

I squashed the commits to avoid merge conflicts and then rebased onto master.

nodejs-github-bot · 2019-07-01T14:09:56Z

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

nodejs-github-bot · 2019-07-05T02:03:18Z

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

yanivfriedensohn · 2019-07-13T14:36:18Z

@Trott the failed tests doesn't seem to be related to the changes of this PR. Can we rerun the CI process?

Trott · 2019-08-08T17:05:30Z

Rebased and force-pushed to get rid of the conflict.

nodejs-github-bot · 2019-08-08T17:44:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24893/ ✔️

Trott · 2019-08-08T18:17:55Z

CITGM (this PR): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1938/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1939/

The above are queued. They will 404 until a worker is available.

Trott · 2019-08-08T18:21:25Z

CI is good and I'll be surprised if CITGM turns up anything. Could use a few more reviews, though. @nodejs/tsc

lib/internal/validators.js

mcollina

LGTM

Trott · 2019-08-09T22:59:25Z

Landed in a0e2c6d.

Thanks for the contribution! 🎉

PR-URL: nodejs#27700 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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

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 May 14, 2019

yanivfriedensohn force-pushed the add-error-codes-to-errors branch from 656e764 to dcfd8a9 Compare May 14, 2019 20:35

sam-github requested review from addaleax and tniessen May 14, 2019 22:37

jasnell approved these changes May 18, 2019

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 18, 2019

joyeecheung reviewed May 20, 2019

View reviewed changes

src/node_buffer.cc Outdated Show resolved Hide resolved

tniessen reviewed May 20, 2019

View reviewed changes

joyeecheung reviewed May 20, 2019

View reviewed changes

src/string_bytes.h Outdated Show resolved Hide resolved

yanivfriedensohn force-pushed the add-error-codes-to-errors branch from 96aa81f to 6b94e80 Compare May 24, 2019 13:37

joyeecheung reviewed May 24, 2019

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

src/string_bytes.cc Outdated Show resolved Hide resolved

yanivfriedensohn force-pushed the add-error-codes-to-errors branch 5 times, most recently from 53ddb82 to 487f8fe Compare May 26, 2019 20:51

joyeecheung reviewed May 27, 2019

View reviewed changes

lib/internal/util.js Outdated Show resolved Hide resolved

joyeecheung approved these changes Jun 5, 2019

View reviewed changes

src/node_crypto.cc Outdated Show resolved Hide resolved

src/node_crypto.cc Outdated Show resolved Hide resolved

src/node_crypto.cc Outdated Show resolved Hide resolved

src/string_bytes.cc Show resolved Hide resolved

joyeecheung approved these changes Jun 7, 2019

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 Jun 27, 2019

yanivfriedensohn force-pushed the add-error-codes-to-errors branch from a612713 to 1404066 Compare June 28, 2019 08:11

This comment has been minimized.

Sign in to view

src: add error codes to errors thrown in C++

9b64d06

Trott force-pushed the add-error-codes-to-errors branch from 1404066 to 9b64d06 Compare August 8, 2019 17:05

mcollina reviewed Aug 9, 2019

View reviewed changes

lib/internal/validators.js Show resolved Hide resolved

mcollina approved these changes Aug 9, 2019

View reviewed changes

Trott approved these changes Aug 9, 2019

View reviewed changes

Trott closed this Aug 9, 2019

yanivfriedensohn deleted the add-error-codes-to-errors branch August 10, 2019 07:59

BethGriggs mentioned this pull request Sep 9, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add error codes to errors thrown in C++ #27700

src: add error codes to errors thrown in C++ #27700

yanivfriedensohn commented May 14, 2019

sam-github commented May 14, 2019

nodejs-github-bot commented May 20, 2019

nodejs-github-bot commented May 20, 2019

tniessen May 20, 2019

joyeecheung May 20, 2019

tniessen May 21, 2019

yanivfriedensohn May 24, 2019

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 28, 2019

yanivfriedensohn commented Jun 4, 2019

joyeecheung left a comment

yanivfriedensohn commented Jun 14, 2019

nodejs-github-bot commented Jun 27, 2019

nodejs-github-bot commented Jun 27, 2019

joyeecheung commented Jun 27, 2019

yanivfriedensohn commented Jun 28, 2019

nodejs-github-bot commented Jul 1, 2019

nodejs-github-bot commented Jul 5, 2019

yanivfriedensohn commented Jul 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Aug 8, 2019

nodejs-github-bot commented Aug 8, 2019 •

edited by Trott

Loading

Trott commented Aug 8, 2019

Trott commented Aug 8, 2019

mcollina left a comment

Trott commented Aug 9, 2019

src: add error codes to errors thrown in C++ #27700

src: add error codes to errors thrown in C++ #27700

Conversation

yanivfriedensohn commented May 14, 2019

Checklist

sam-github commented May 14, 2019

nodejs-github-bot commented May 20, 2019

nodejs-github-bot commented May 20, 2019

tniessen May 20, 2019

Choose a reason for hiding this comment

joyeecheung May 20, 2019

Choose a reason for hiding this comment

tniessen May 21, 2019

Choose a reason for hiding this comment

yanivfriedensohn May 24, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented May 27, 2019

nodejs-github-bot commented May 28, 2019

yanivfriedensohn commented Jun 4, 2019

joyeecheung left a comment

Choose a reason for hiding this comment

yanivfriedensohn commented Jun 14, 2019

nodejs-github-bot commented Jun 27, 2019

nodejs-github-bot commented Jun 27, 2019

joyeecheung commented Jun 27, 2019

yanivfriedensohn commented Jun 28, 2019

nodejs-github-bot commented Jul 1, 2019

nodejs-github-bot commented Jul 5, 2019

yanivfriedensohn commented Jul 13, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Aug 8, 2019

nodejs-github-bot commented Aug 8, 2019 • edited by Trott Loading

Trott commented Aug 8, 2019

Trott commented Aug 8, 2019

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Aug 9, 2019

nodejs-github-bot commented Aug 8, 2019 •

edited by Trott

Loading