src: fix error reporting on CPUUsage#34762

yashLadha · 2020-08-13T17:43:55Z

Currently we are returning the stringified error code while in other
process methods we are throwin a UVException and only exclusion is in
the CPUUsage. Converted it to follow the convention, which is being written in the
file.

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

mmarchini · 2020-08-14T05:41:10Z

Is this semver-major (since now we're throwing instead of returning an error string) or is this change not observable by the user?

nodejs-github-bot · 2020-08-14T05:43:42Z

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

tniessen

If we want to do this, the change should at least be semver-major since it changes the error code.

Also, if we want to do this, the previous error code should be removed from /lib, and the documentation in /doc should be updated. Error handling in /lib for cpuUsage should be removed.

But that's a big if. Does this actually benefit users? How likely is it that the UV exception contains more useful information?

mmarchini · 2020-08-17T18:53:01Z

@yashLadha can you share the error messages and error code before and after this commit?

mmarchini · 2020-08-17T19:05:05Z

But that's a big if. Does this actually benefit users? How likely is it that the UV exception contains more useful information?

FWIW if the information is the same (and not reduced), but this PR makes it so that this error is following the convention of other libuv errors, I'm still +1 on this change.

addaleax · 2020-08-18T11:43:14Z

I'm agreeing with @mmarchini. However, as @tniessen mentions, this needs to come with the corresponding changes to the documentation and the JS source.

yashLadha · 2020-08-18T11:46:47Z

@addaleax not sure because we haven't documented these errors for other methods as well. However, it will be document for this as well as others if needed and will be a good addition.

tniessen · 2020-08-18T14:26:10Z

not sure because we haven't documented these errors for other methods as well. However, it will be document for this as well as others if needed and will be a good addition.

@yashLadha It's not about documenting what errors can be thrown, but rather documenting the error codes. In particular, your change would require moving ERR_CPU_USAGE to the legacy error codes further down in that document.

The error code should also be removed from the code base, and error handling in JS should be removed.

yashLadha · 2020-08-19T07:11:08Z

Yes, @tniessen acked on your suggestion.

Trott · 2020-08-23T11:26:03Z

@tniessen Are the changes that were made sufficient to address your concerns? Or are you -1 on this?

doc/api/errors.md

tniessen

Thanks for the ping @Trott :)

nodejs-github-bot · 2020-08-31T18:53:26Z

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

aduh95 · 2020-10-16T08:08:44Z

@yashLadha this needs a rebase.

Currently we are returning the stringified error code while in other process methods we are throwin a UVException and only exclusion is in the CPUUsage. Converted it to follow the convention.

Co-authored-by: Anna Henningsen <[email protected]>

yashLadha · 2020-10-16T08:32:41Z

Resolved the conflict @aduh95

nodejs-github-bot · 2020-10-16T08:42:41Z

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

nodejs-github-bot · 2020-10-16T12:18:34Z

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

Currently we are returning the stringified error code while in other process methods we are throwin a UVException and only exclusion is in the CPUUsage. Converted it to follow the convention. PR-URL: #34762 Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Zeyu Yang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

aduh95 · 2020-10-16T22:46:05Z

Landed in eeb6b47

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 13, 2020

yashLadha force-pushed the fix_error_response_cpuUsage branch from dfaee4b to 98b30d7Compare August 13, 2020 17:44

jasnell approved these changes Aug 13, 2020
View reviewed changes

devnexen approved these changes Aug 13, 2020
View reviewed changes

mmarchini approved these changes Aug 14, 2020
View reviewed changes

mmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2020

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2020

himself65 approved these changes Aug 14, 2020
View reviewed changes

Trott approved these changes Aug 15, 2020
View reviewed changes

BridgeAR changed the title ~~fix: error reporting on CPUUsage~~src: fix error reporting on CPUUsageAug 16, 2020

tniessen requested changes Aug 17, 2020
View reviewed changes

mmarchini added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2020

yashLadha force-pushed the fix_error_response_cpuUsage branch from 98b30d7 to 45e2945Compare August 19, 2020 07:10

yashLadha force-pushed the fix_error_response_cpuUsage branch 3 times, most recently from 03f86b1 to 0fecf1bCompare August 20, 2020 06:51

addaleax approved these changes Aug 23, 2020
View reviewed changes

doc/api/errors.md Outdated Show resolvedHide resolved

tniessen approved these changes Aug 23, 2020
View reviewed changes

mmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2020

mmarchini approved these changes Aug 31, 2020
View reviewed changes

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2020

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2020

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2020

yashLadhaand others added 2 commits October 16, 2020 14:01

src: error reporting on CPUUsage
0552c15
Currently we are returning the stringified error code while in other process methods we are throwin a UVException and only exclusion is in the CPUUsage. Converted it to follow the convention.

Update doc/api/errors.md
4a54765
Co-authored-by: Anna Henningsen <[email protected]>

yashLadha force-pushed the fix_error_response_cpuUsage branch from ac7070e to 4a54765Compare October 16, 2020 08:32

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 16, 2020

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

gireeshpunathil approved these changes Oct 16, 2020
View reviewed changes

aduh95 closed this Oct 16, 2020

Uh oh!

src: fix error reporting on CPUUsage#34762

src: fix error reporting on CPUUsage #34762

Uh oh!

Conversation

yashLadha commented Aug 13, 2020

Checklist

Uh oh!

mmarchini commented Aug 14, 2020

Uh oh!

nodejs-github-bot commented Aug 14, 2020

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

mmarchini commented Aug 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmarchini commented Aug 17, 2020

Uh oh!

addaleax commented Aug 18, 2020

Uh oh!

yashLadha commented Aug 18, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Aug 18, 2020

Uh oh!

yashLadha commented Aug 19, 2020

Uh oh!

Trott commented Aug 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 31, 2020

Uh oh!

aduh95 commented Oct 16, 2020

Uh oh!

yashLadha commented Oct 16, 2020

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

aduh95 commented Oct 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mmarchini commented Aug 17, 2020•
edited
Loading

yashLadha commented Aug 18, 2020•
edited
Loading

Trott commented Aug 23, 2020•
edited
Loading