uv: improvements to process.binding('uv')

* 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>
nodejs · Aug 23, 2017 · 58831b2 · 58831b2
1 parent 5e7c697
commit 58831b2
Show file tree

Hide file tree

Showing 10 changed files with 102 additions and 39 deletions.
diff --git a/lib/_stream_wrap.js b/lib/_stream_wrap.js
@@ -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);
     });

diff --git a/lib/child_process.js b/lib/child_process.js
@@ -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;
@@ -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;
     }
 
@@ -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;

diff --git a/lib/dns.js b/lib/dns.js
@@ -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,
@@ -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;

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
@@ -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;
@@ -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;
@@ -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++) {
@@ -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 {

diff --git a/lib/internal/cluster/round_robin_handle.js b/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);
   });
 };
 

diff --git a/lib/net.js b/lib/net.js
@@ -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');
@@ -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'));
   }
 
@@ -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;
@@ -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) {

diff --git a/lib/util.js b/lib/util.js
@@ -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;
 }

diff --git a/src/uv.cc b/src/uv.cc
@@ -29,7 +29,6 @@ namespace {
 
 using v8::Context;
 using v8::FunctionCallbackInfo;
-using v8::Integer;
 using v8::Local;
 using v8::Object;
 using v8::Value;
@@ -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));
 }
@@ -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
 }

diff --git a/test/parallel/test-uv-binding-constant.js b/test/parallel/test-uv-binding-constant.js
@@ -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);
+});
diff --git a/test/parallel/test-uv-errno.js b/test/parallel/test-uv-errno.js
@@ -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'
+    });
+});