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

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

- 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:
consterrCode=(()=>{if(!err)return0;returnuv[`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

- 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:
consterrCode=(()=>{if(!err)return0;returnuv[`UV_${err.code}`]||uv.UV_EPIPE;})();

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

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

+});
+
+[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

+ 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
{
letthrew=false;
try{
assert.doesNotThrow(makeBlock(thrower,Error));
}catch(e){
threw=true;
common.expectsError({
code: 'ERR_ASSERTION',
message: /Gotunwantedexception\.\n\[objectObject\]/
})(e);
}
assert.ok(threw);
}

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

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

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


Personally I'd go:
constkeys=Object.keys(uv).filter(k=>k!=='errname');
and drop L10-11

Not particularly a fan of using filter

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

+ 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

- 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

- 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

	{
	letthrew=false;
	try{
	assert.doesNotThrow(makeBlock(thrower,Error));
	}catch(e){
	threw=true;
	common.expectsError({
	code: 'ERR_ASSERTION',
	message: /Gotunwantedexception\.\n\[objectObject\]/
	})(e);
	}
	assert.ok(threw);
	}

Uh oh!

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!

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

jasnellAug 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

refackAug 19, 2017•
edited
Loading

jasnellAug 20, 2017•
edited
Loading