src, os, dgram, tty: migrate to internal errors #16567

jasnell · 2017-10-28T06:32:37Z

Adds mechanism for transitioning internal UV and ErrnoException to internal/errors
Migrates os, tty, and dgram errors to internal/errors

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)

src, os, dgram, tty

jasnell · 2017-10-28T06:33:16Z

ping @nodejs/tsc

jasnell · 2017-10-28T06:42:31Z

That wouldn't catch cases like new SystemError(null)

targos · 2017-10-28T06:50:25Z

Can't that case be changed to pass undefined?

targos · 2017-10-28T07:03:49Z

lib/internal/errors.js

+    Object.defineProperty(this, kInfo, {
+      configurable: false,
+      enumerable: false,
+      value: context || {}


context is guaranteed to be truthy here

maclover7 · 2017-10-28T13:46:06Z

This is really great! Should any docs be added on the C++ side about the new way to throw errors... maybe in the style guide?

jasnell · 2017-10-29T16:55:02Z

@targos

Can't that case be changed to pass undefined

potentially. Let's save that particular change for later tho. Once I get through converting all of the native level code to use this I will verify that we can safely make that assumption.

@maclover7

Should any docs be added on the C++ side about the new way to throw errors

Yes, I'm planning to write up a more detailed guide on this once I'm through the initial conversion.

joyeecheung · 2017-10-30T08:37:29Z

doc/api/errors.md

+
+### error.info
+
+`SystemError` instances may have an additional `info` property whose


Come to think of it, I think a more popular pattern in the userland is using the err.data property (correct me if I am wrong), I remember seeing some logging services/tools collect that automatically in addition to err.code...I remember there is another PR for crypto that use info. Is there a reason that we pick info instead of data?

A quick search in our own code base shows that eslint is using error.data: https://github.com/nodejs/node/blob/master/tools/eslint/lib/config/config-validator.js#L92 and https://github.com/nodejs/node/blob/master/tools/eslint/lib/config/config-validator.js#L179

I've seen both: https://www.npmjs.com/package/verror uses *.info

On a side note: would be easier to read if we have a syntax for documenting optional properties..

I agree, definitely something worth working on

joyeecheung

Changes LGTM, left a few suggestions.

joyeecheung · 2017-10-30T16:04:50Z

lib/dgram.js

+  const ctx = {};
+  const ret = self._handle.bufferSize(size, buffer, ctx);
+  if (ret === undefined) {
+    throw new errors.Error('ERR_SOCKET_BUFFER_SIZE',


Is this for avoiding changing the error type? Maybe a comment because this looks a bit odd at first?

Yes, I really dislike how this was done, but I want to save it for a different PR to change it.

joyeecheung · 2017-10-30T16:13:31Z

lib/os.js

+    const ctx = {};
+    const ret = fn(...args, ctx);
+    if (ret === undefined) {
+      throw new errors.SystemError(ctx);


Maybe name this function and do a captureStackTrace()?

joyeecheung · 2017-10-30T16:27:00Z

src/node_os.cc

@@ -351,7 +356,8 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
  const int err = uv_os_get_passwd(&pwd);

  if (err) {
-    return env->ThrowUVException(err, "uv_os_get_passwd");
+    env->CollectUVExceptionInfo(args[1], err, "uv_os_get_passwd");


For clarity, maybe use args[args.Length() - 1]? Also we should probably CHECK_EQ(args.Length(), n) or CHECK_GE(args.Length(), n) (not sure if these are used in userland)

joyeecheung · 2017-10-30T16:42:09Z

test/parallel/test-errors-systemerror.js

+    dest: Buffer.from('b')
+  };
+  const err = new errors.SystemError(ctx);
+  assert.strictEqual(err.info, ctx);


This one is not legacy, right? Should we move the comment below this?

Preparing for the migration of existing UVException and ErrnoExceptions from the native layer, add new `errors.SystemError` to internal/errors and new `env->CollectExceptionInfo()` / `env->CollectUVExceptionInfo()` methods.

jasnell · 2017-11-02T05:18:35Z

@targos @joyeecheung ... updated, PTAL

mhdawson

LGTM as long as CI is green.

mhdawson · 2017-11-02T14:10:06Z

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

jasnell · 2017-11-02T18:27:05Z

CI is good. failures are unrelated and known flakies

jasnell · 2017-11-02T18:30:12Z

(getting this landed)

Preparing for the migration of existing UVException and ErrnoExceptions from the native layer, add new `errors.SystemError` to internal/errors and new `env->CollectExceptionInfo()` / `env->CollectUVExceptionInfo()` methods. PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell · 2017-11-02T19:01:42Z

Landed in c3dc0e0, 056b858, 3d9d849, and 9ad994b

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 28, 2017

targos reviewed Oct 28, 2017

View reviewed changes

jasnell mentioned this pull request Oct 30, 2017

crypto, errors: Add CryptoError to internal/errors, update crypto.setEngine #16529

Closed

4 tasks

jasnell force-pushed the migrate-os-internal-errors branch from 6350e83 to 4a8cbd3 Compare October 30, 2017 01:06

joyeecheung reviewed Oct 30, 2017

View reviewed changes

joyeecheung approved these changes Oct 30, 2017

View reviewed changes

jasnell added 4 commits November 1, 2017 21:55

src: add CollectExceptionInfo & errors.SystemError

7462524

Preparing for the migration of existing UVException and ErrnoExceptions from the native layer, add new `errors.SystemError` to internal/errors and new `env->CollectExceptionInfo()` / `env->CollectUVExceptionInfo()` methods.

os: migrate node_os.cc to internal/errors

5698a33

tty: convert to internal/errors using SystemError

1863099

dgram: migrate bufferSize to use internal/errors

ee9e7a6

jasnell force-pushed the migrate-os-internal-errors branch from 4a8cbd3 to ee9e7a6 Compare November 2, 2017 05:17

joyeecheung approved these changes Nov 2, 2017

View reviewed changes

mhdawson approved these changes Nov 2, 2017

View reviewed changes

jasnell added a commit that referenced this pull request Nov 2, 2017

os: migrate node_os.cc to internal/errors

056b858

PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Nov 2, 2017

tty: convert to internal/errors using SystemError

3d9d849

PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Nov 2, 2017

dgram: migrate bufferSize to use internal/errors

9ad994b

PR-URL: #16567 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell closed this Nov 2, 2017

MylesBorins mentioned this pull request Dec 11, 2017

src: explicitly register built-in modules #16565

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src, os, dgram, tty: migrate to internal errors #16567

src, os, dgram, tty: migrate to internal errors #16567

jasnell commented Oct 28, 2017

jasnell commented Oct 28, 2017

jasnell commented Oct 28, 2017

targos commented Oct 28, 2017

targos Oct 28, 2017

maclover7 commented Oct 28, 2017

jasnell commented Oct 29, 2017

joyeecheung Oct 30, 2017 •

edited

Loading

joyeecheung Oct 30, 2017

jasnell Oct 30, 2017

joyeecheung Oct 30, 2017

jasnell Nov 2, 2017

joyeecheung left a comment

joyeecheung Oct 30, 2017

jasnell Oct 31, 2017

joyeecheung Oct 30, 2017

joyeecheung Oct 30, 2017

joyeecheung Oct 30, 2017

jasnell commented Nov 2, 2017

mhdawson left a comment

mhdawson commented Nov 2, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017


		### error.info

		`SystemError` instances may have an additional `info` property whose

src, os, dgram, tty: migrate to internal errors #16567

src, os, dgram, tty: migrate to internal errors #16567

Conversation

jasnell commented Oct 28, 2017

Checklist

Affected core subsystem(s)

jasnell commented Oct 28, 2017

jasnell commented Oct 28, 2017

targos commented Oct 28, 2017

Choose a reason for hiding this comment

maclover7 commented Oct 28, 2017

jasnell commented Oct 29, 2017

joyeecheung Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 2, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Nov 2, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

jasnell commented Nov 2, 2017

joyeecheung Oct 30, 2017 •

edited

Loading