uv: binding improvements #14933

jasnell · 2017-08-18T23:35:19Z

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.

addaleax · 2017-08-18T23:38:40Z

lib/_stream_wrap.js

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


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; })();

addaleax · 2017-08-19T00:32:50Z

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

refack · 2017-08-19T00:42:32Z

lib/_stream_wrap.js

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


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; })();

refack · 2017-08-19T00:44:12Z

lib/internal/cluster/round_robin_handle.js

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


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

refack · 2017-08-19T00:48:40Z

test/parallel/test-uv-errno.js

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


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' } );

refack · 2017-08-19T00:50:02Z

test/parallel/test-uv-errno.js

+  if (key === 'errname')
+    return;
+
+  assert.doesNotThrow(() => {


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

refack · 2017-08-19T00:51:47Z

test/parallel/test-uv-errno.js

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


Personally I'd go:

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

and drop L10-11

Not particularly a fan of using filter

refack · 2017-08-19T00:53:08Z

test/parallel/test-uv-binding-constant.js

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


~~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.

refack · 2017-08-19T00:54:47Z

Not all comments are blocking, but PTAL.

jasnell · 2017-08-19T00:54:54Z

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

refack · 2017-08-19T01:45:45Z

lib/util.js

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


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

Comments have been discussed.

jasnell · 2017-08-19T04:00:09Z

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

lpinca · 2017-08-19T07:25:07Z

lib/util.js

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


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.

jasnell · 2017-08-21T18:21:20Z

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2017-08-23T17:54:26Z

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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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

	{
	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);
	}

Uh oh!

Uh oh!

uv: binding improvements #14933

uv: binding improvements #14933

Uh oh!

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

refack Aug 19, 2017 •

edited

Loading

jasnell Aug 20, 2017 •

edited

Loading