Skip to content

uv: binding improvements #14933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jasnell wants to merge 4 commits into from
Closed

uv: binding improvements #14933

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01fea72
uv: ensure that UV_... props are constants
jasnell Aug 18, 2017
4527a9a
uv: improve error and guard against abort
jasnell Aug 18, 2017
6a0422d
lib: refine uses of process.binding('uv')
jasnell Aug 18, 2017
bd80a66
[Squash] Address nit
jasnell Aug 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
uv: improve error and guard against abort 
The error message when passing in `err >= 0` to `util._errnoException()`
was less than useful.
  • Loading branch information
@jasnell
jasnell committed Aug 18, 2017
commit 4527a9ab3e21153b6042ba0483c96b348b818cb9
11 changes: 7 additions & 4 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,13 +1030,16 @@ function error(...args) {
}

function _errnoException(err, syscall, original) {
var name = errname(err);
if (typeof err !== 'number' || err >= 0 || !Number.isSafeInteger(err)) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'err',
'negative number');
Copy link
Copy Markdown
Member

@lpinca lpinca Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add the actual argument?

Copy link
Copy Markdown
Member Author

@jasnell jasnell Aug 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const name = errname(err);
var message = `${syscall} ${name}`;
if (original)
message += ` ${original}`;
var e = new Error(message);
e.code = name;
e.errno = name;
const e = new Error(message);
Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@jasnell jasnell Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

e.code = e.errno = name;
e.syscall = syscall;
return e;
}
Expand Down
3 changes: 1 addition & 2 deletions src/uv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ using v8::Value;
void ErrName(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
int err = args[0]->Int32Value();
if (err >= 0)
return env->ThrowError("err >= 0");
CHECK_LT(err, 0);
const char* name = uv_err_name(err);
args.GetReturnValue().Set(OneByteString(env->isolate(), name));
}
Expand Down
30 changes: 30 additions & 0 deletions test/parallel/test-uv-errno.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const util = require('util');
const uv = process.binding('uv');

const keys = Object.keys(uv);
Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'd go:

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

and drop L10-11

Copy link
Copy Markdown
Member Author

@jasnell jasnell Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not particularly a fan of using filter

Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.


keys.forEach((key) => {
if (key === 'errname')
return;

assert.doesNotThrow(() => {
Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@jasnell jasnell Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary but not a problem either

Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@jasnell jasnell Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const err = util._errnoException(uv[key], 'test');
const name = uv.errname(uv[key]);
assert.strictEqual(err.code, err.errno);
assert.strictEqual(err.code, name);
assert.strictEqual(err.message, `test ${name}`);
});
});

[0, 1, 'test', {}, [], Infinity, -Infinity, NaN].forEach((key) => {
assert.throws(() => util._errnoException(key),
Copy link
Copy Markdown
Contributor

@refack refack Aug 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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