Skip to content

Commit

Permalink
uv: improvements to process.binding('uv')
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
jasnell committed Aug 23, 2017
1 parent 5e7c697 commit 58831b2
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 39 deletions.
14 changes: 6 additions & 8 deletions lib/_stream_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,18 @@ StreamWrap.prototype.doWrite = function doWrite(req, bufs) {
// Ensure that this is called once in case of error
pending = 0;

let errCode = 0;
if (err) {
const code = uv[`UV_${err.code}`];
errCode = (err.code && code) ? code : uv.UV_EPIPE;
}

// Ensure that write was dispatched
setImmediate(function() {
// Do not invoke callback twice
if (!self._dequeue(item))
return;

var errCode = 0;
if (err) {
if (err.code && uv['UV_' + err.code])
errCode = uv['UV_' + err.code];
else
errCode = uv.UV_EPIPE;
}

handle.doAfterWrite(req);
handle.finishWrite(req, errCode);
});
Expand Down
6 changes: 3 additions & 3 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ const { createPromise,
promiseResolve, promiseReject } = process.binding('util');
const debug = util.debuglog('child_process');

const uv = process.binding('uv');
const Buffer = require('buffer').Buffer;
const Pipe = process.binding('pipe_wrap').Pipe;
const { isUint8Array } = process.binding('util');
const { errname } = process.binding('uv');
const child_process = require('internal/child_process');

const _validateStdio = child_process._validateStdio;
Expand Down Expand Up @@ -267,7 +267,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
if (!ex) {
ex = new Error('Command failed: ' + cmd + '\n' + stderr);
ex.killed = child.killed || killed;
ex.code = code < 0 ? uv.errname(code) : code;
ex.code = code < 0 ? errname(code) : code;
ex.signal = signal;
}

Expand Down Expand Up @@ -565,7 +565,7 @@ function checkExecSyncError(ret, args, cmd) {
err = new Error(msg);
}
if (err) {
err.status = ret.status < 0 ? uv.errname(ret.status) : ret.status;
err.status = ret.status < 0 ? errname(ret.status) : ret.status;
err.signal = ret.signal;
}
return err;
Expand Down
12 changes: 8 additions & 4 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,14 @@
const util = require('util');

const cares = process.binding('cares_wrap');
const uv = process.binding('uv');
const internalNet = require('internal/net');
const { customPromisifyArgs } = require('internal/util');
const errors = require('internal/errors');
const {
UV_EAI_MEMORY,
UV_EAI_NODATA,
UV_EAI_NONAME
} = process.binding('uv');

const {
GetAddrInfoReqWrap,
Expand All @@ -43,9 +47,9 @@ const isLegalPort = internalNet.isLegalPort;
function errnoException(err, syscall, hostname) {
// FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
// the true error to the user. ENOTFOUND is not even a proper POSIX error!
if (err === uv.UV_EAI_MEMORY ||
err === uv.UV_EAI_NODATA ||
err === uv.UV_EAI_NONAME) {
if (err === UV_EAI_MEMORY ||
err === UV_EAI_NODATA ||
err === UV_EAI_NONAME) {
err = 'ENOTFOUND';
}
var ex = null;
Expand Down
25 changes: 17 additions & 8 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const assert = require('assert');

const Process = process.binding('process_wrap').Process;
const WriteWrap = process.binding('stream_wrap').WriteWrap;
const uv = process.binding('uv');
const Pipe = process.binding('pipe_wrap').Pipe;
const TTY = process.binding('tty_wrap').TTY;
const TCP = process.binding('tcp_wrap').TCP;
Expand All @@ -20,6 +19,16 @@ const { isUint8Array } = process.binding('util');
const { convertToValidSignal } = require('internal/util');
const spawn_sync = process.binding('spawn_sync');

const {
UV_EAGAIN,
UV_EINVAL,
UV_EMFILE,
UV_ENFILE,
UV_ENOENT,
UV_ENOSYS,
UV_ESRCH
} = process.binding('uv');

const errnoException = util._errnoException;
const SocketListSend = SocketList.SocketListSend;
const SocketListReceive = SocketList.SocketListReceive;
Expand Down Expand Up @@ -307,17 +316,17 @@ ChildProcess.prototype.spawn = function(options) {
var err = this._handle.spawn(options);

// Run-time errors should emit an error, not throw an exception.
if (err === uv.UV_EAGAIN ||
err === uv.UV_EMFILE ||
err === uv.UV_ENFILE ||
err === uv.UV_ENOENT) {
if (err === UV_EAGAIN ||
err === UV_EMFILE ||
err === UV_ENFILE ||
err === UV_ENOENT) {
process.nextTick(onErrorNT, this, err);
// There is no point in continuing when we've hit EMFILE or ENFILE
// because we won't be able to set up the stdio file descriptors.
// It's kind of silly that the de facto spec for ENOENT (the test suite)
// mandates that stdio _is_ set up, even if there is no process on the
// receiving end, but it is what it is.
if (err !== uv.UV_ENOENT) return err;
if (err !== UV_ENOENT) return err;
} else if (err) {
// Close all opened fds on error
for (i = 0; i < stdio.length; i++) {
Expand Down Expand Up @@ -394,9 +403,9 @@ ChildProcess.prototype.kill = function(sig) {
this.killed = true;
return true;
}
if (err === uv.UV_ESRCH) {
if (err === UV_ESRCH) {
/* Already dead. */
} else if (err === uv.UV_EINVAL || err === uv.UV_ENOSYS) {
} else if (err === UV_EINVAL || err === UV_ENOSYS) {
/* The underlying platform doesn't support this signal. */
throw errnoException(err, 'kill');
} else {
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/cluster/round_robin_handle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
};

Expand Down
12 changes: 8 additions & 4 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ const internalUtil = require('internal/util');
const internalNet = require('internal/net');
const assert = require('assert');
const cares = process.binding('cares_wrap');
const uv = process.binding('uv');
const {
UV_EADDRINUSE,
UV_EINVAL,
UV_EOF
} = process.binding('uv');

const Buffer = require('buffer').Buffer;
const TTYWrap = process.binding('tty_wrap');
Expand Down Expand Up @@ -604,7 +608,7 @@ function onread(nread, buffer) {
}

// Error, possibly EOF.
if (nread !== uv.UV_EOF) {
if (nread !== UV_EOF) {
return self.destroy(errnoException(nread, 'read'));
}

Expand Down Expand Up @@ -1229,7 +1233,7 @@ function createServerHandle(address, port, addressType, fd) {
} catch (e) {
// Not a fd we can listen on. This will trigger an error.
debug('listen invalid fd=%d:', fd, e.message);
return uv.UV_EINVAL;
return UV_EINVAL;
}
handle.open(fd);
handle.readable = true;
Expand Down Expand Up @@ -1389,7 +1393,7 @@ function listenInCluster(server, address, port, addressType,
var out = {};
err = handle.getsockname(out);
if (err === 0 && port !== out.port)
err = uv.UV_EADDRINUSE;
err = UV_EADDRINUSE;
}

if (err) {
Expand Down
11 changes: 7 additions & 4 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -1031,13 +1031,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');
}
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);
e.code = e.errno = name;
e.syscall = syscall;
return e;
}
Expand Down
9 changes: 3 additions & 6 deletions src/uv.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ namespace {

using v8::Context;
using v8::FunctionCallbackInfo;
using v8::Integer;
using v8::Local;
using v8::Object;
using v8::Value;
Expand All @@ -38,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 All @@ -51,9 +49,8 @@ void InitializeUV(Local<Object> target,
Environment* env = Environment::GetCurrent(context);
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "errname"),
env->NewFunctionTemplate(ErrName)->GetFunction());
#define V(name, _) \
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "UV_" # name), \
Integer::New(env->isolate(), UV_ ## name));

#define V(name, _) NODE_DEFINE_CONSTANT(target, UV_##name);
UV_ERRNO_MAP(V)
#undef V
}
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-uv-binding-constant.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

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

// Ensures that the `UV_...` values in process.binding('uv')
// are constants.

const keys = Object.keys(uv);
keys.forEach((key) => {
if (key === 'errname')
return; // skip this
const val = uv[key];
assert.throws(() => uv[key] = 1,
/^TypeError: Cannot assign to read only property/);
assert.strictEqual(uv[key], val);
});
31 changes: 31 additions & 0 deletions test/parallel/test-uv-errno.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

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

const keys = Object.keys(uv);

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

assert.doesNotThrow(() => {
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) => {
common.expectsError(
() => util._errnoException(key),
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "err" argument must be of type negative number'
});
});

0 comments on commit 58831b2

Please sign in to comment.