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

@@ -56,8 +56,7 @@ RoundRobinHandle.prototype.add = function(worker, send) {
    // Hack: translate 'EADDRINUSE' error string back to numeric error code.
    // It works but ideally we'd have some backchannel between the net and
    // 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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uv: binding improvements #14933

uv: binding improvements #14933

jasnell commented Aug 18, 2017

addaleax Aug 18, 2017

refack Aug 19, 2017

addaleax commented Aug 19, 2017

refack Aug 19, 2017

refack Aug 19, 2017

refack Aug 19, 2017

refack Aug 19, 2017

jasnell Aug 19, 2017

refack Aug 19, 2017

jasnell Aug 19, 2017

refack Aug 19, 2017

jasnell Aug 19, 2017

refack Aug 19, 2017

refack Aug 19, 2017 •

edited

Loading

refack Aug 19, 2017

refack commented Aug 19, 2017

jasnell commented Aug 19, 2017

refack Aug 19, 2017

jasnell Aug 19, 2017

refack Aug 19, 2017

jasnell commented Aug 19, 2017

lpinca Aug 19, 2017

jasnell Aug 20, 2017 •

edited

Loading

jasnell commented Aug 21, 2017

jasnell commented Aug 23, 2017

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

uv: binding improvements #14933

uv: binding improvements #14933

Conversation

jasnell commented Aug 18, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 19, 2017

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

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

refack Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 19, 2017

jasnell commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 19, 2017

Choose a reason for hiding this comment

jasnell Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Aug 21, 2017

jasnell commented Aug 23, 2017

refack Aug 19, 2017 •

edited

Loading

jasnell Aug 20, 2017 •

edited

Loading