uv: binding improvements by jasnell · Pull Request #14933 · nodejs/node · GitHub

Few improvements to process.binding('uv')

Checklist

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

Affected core subsystem(s)

uv

The error message when passing in `err >= 0` to `util._errnoException()` was less than useful.

teeny tiny suggestion: err.code && code in parentheses might be more readable

Suggestions:

This closes over err, why not hoist it out of the callback, so only errCode gets closed.
if !err.code then `UV_${err.code}` == somethinglike('UV_undefined') so uv[`UV_${err.code}`] == false
IIFE can be nice:

const errCode = (() => {
  if (!err) return 0;
  return uv[`UV_${err.code}`] || uv.UV_EPIPE;
})();

I don’t think this needs to be semver-major.

Suggestions:

This closes over err, why not hoist it out of the callback, so only errCode gets closed.
if !err.code then `UV_${err.code}` == somethinglike('UV_undefined') so uv[`UV_${err.code}`] == false
IIFE can be nice:

const errCode = (() => {
  if (!err) return 0;
  return uv[`UV_${err.code}`] || uv.UV_EPIPE;
})();

-0 on this.
I like one expression per line...

You can use the new expectsError syntex:

common.expectsError(
  () => util._errnoException(key),
  {
    code: 'ERR_INVALID_ARG_TYPE',
    type: TypeError,
    message: 'The "err" argument must be of type negative number'
  }
);

I'm not sure the assert.doesNotThrow is necessary...

Not strictly necessary but not a problem either

I think it mangles the actual error a little bit:

node/test/parallel/test-assert.js

Lines 485 to 497 in 467385a

    
           { 
        
             let threw = false; 
        
             try { 
        
               assert.doesNotThrow(makeBlock(thrower, Error)); 
        
             } catch (e) { 
        
               threw = true; 
        
               common.expectsError({ 
        
                 code: 'ERR_ASSERTION', 
        
                 message: /Got unwanted exception\.\n\[object Object\]/ 
        
               })(e); 
        
             } 
        
             assert.ok(threw); 
        
           }

I'm aware of what the function does. These cases do not throw

Personally I'd go:

const keys = Object.keys(uv).filter(k => k !== 'errname');

and drop L10-11

Not particularly a fan of using filter

Ack.

AFAICT assignment is not a valid lambda expression, you need to () => { uv[key] = 1; }

So what am I thinking us 😕? I remember an old debate that ended up with the invention of Object.assign().
Ohh it return the LHS, not the object.

Not all comments are blocking, but PTAL.

It has to be semver-major because of the error message change.

IMHO a new type something like errors.UVError or errors.PlatformError would be nice, but then we'd need to expose it somehow.

That's not something I want to do in this pr

Agreed.

Comments have been discussed.

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

Does it make sense to add the actual argument?

Not particularly useful. The ERR_INVALID_ARG_TYPE actual prints out the type of the actual, not the value itself. So if I passed in _errnoException(1), the error message would be The "err" argument must be of type negative number. Received type number, which is just odd. We could turn this into a RangeError if the err is a number and isn't negative, but that adds a second if-check that is not strictly necessary for what is supposed to be an internal API.

CI run was a bit too red, although all of the failures were unrelated. Running again just to be safe: https://ci.nodejs.org/job/node-test-pull-request/9778/

* ensure that UV_... props are constants * improve error message ... the error message when passing in `err >= 0` to `util._errnoException()` was less than useful. * refine uses of process.binding('uv') PR-URL: #14933 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed in 58831b2

* ensure that UV_... props are constants * improve error message ... the error message when passing in `err >= 0` to `util._errnoException()` was less than useful. * refine uses of process.binding('uv') PR-URL: nodejs/node#14933 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added 3 commits August 18, 2017 16:00

uv: ensure that UV_... props are constants

01fea72

uv: improve error and guard against abort

4527a9a

The error message when passing in `err >= 0` to `util._errnoException()` was less than useful.

lib: refine uses of process.binding('uv')

6a0422d

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

jasnell requested review from addaleax and cjihrig August 18, 2017 23:35

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

addaleax approved these changes Aug 18, 2017

View reviewed changes

refack previously requested changes Aug 19, 2017

View reviewed changes

refack reviewed Aug 19, 2017

View reviewed changes

mscdex added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Aug 19, 2017

[Squash] Address nit

bd80a66

refack approved these changes Aug 19, 2017

View reviewed changes

lpinca reviewed Aug 19, 2017

View reviewed changes

cjihrig approved these changes Aug 19, 2017

View reviewed changes

jasnell closed this Aug 23, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

-                      else
-                        errCode = uv.UV_EPIPE;
+                      const code = uv[`UV_${err.code}`];
+                      errCode = err.code && code ? code : uv.UV_EPIPE;

-                      else
-                        errCode = uv.UV_EPIPE;
+                      const code = uv[`UV_${err.code}`];
+                      errCode = err.code && code ? code : uv.UV_EPIPE;

                   // cluster modules for stuff like this.
-                  const errno = uv['UV_' + err.errno];
-                  send(errno, null);
+                  send(uv[`UV_${err.errno}`], null);

+                if (key === 'errname')
+                  return; // skip this
+                const val = uv[key];
+                assert.throws(() => uv[key] = 1,

-                var e = new Error(message);
-                e.code = name;
-                e.errno = name;
+                const e = new Error(message);

+              });
+              [0, 1, 'test', {}, [], Infinity, -Infinity, NaN].forEach((key) => {
+                assert.throws(() => util._errnoException(key),

	{
	let threw = false;
	try {
	assert.doesNotThrow(makeBlock(thrower, Error));
	} catch (e) {
	threw = true;
	common.expectsError({
	code: 'ERR_ASSERTION',
	message: /Got unwanted exception\.\n\[object Object\]/
	})(e);
	}
	assert.ok(threw);
	}

+              const util = require('util');
+              const uv = process.binding('uv');
+              const keys = Object.keys(uv);

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

uv: binding improvements by jasnell · Pull Request #14933 · nodejs/node · GitHub

Uh oh!

Uh oh!

uv: binding improvements - #14933

uv: binding improvements#14933jasnell wants to merge 4 commits intonodejs:masternodejs/node:masterfrom jasnell:uv-binding-improvementsjasnell/node:uv-binding-improvementsCopy head branch name to clipboard

Conversation

jasnell commented Aug 18, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 19, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

refack Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 19, 2017

Uh oh!

jasnell commented Aug 19, 2017

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!

jasnell commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 21, 2017

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

uv: binding improvements#14933
jasnell wants to merge 4 commits into
nodejs:masterfrom
jasnell:uv-binding-improvements

refack Aug 19, 2017 •
edited
Loading

jasnell Aug 20, 2017 •
edited
Loading