net: use actual Timeout instance on Sockets

This makes `net.Sockets` use actual Timeout objects in a `[kTimeout]`
symbol property, rather than making the socket itself a timer and
appending properties to it directly.

This should make the code generally easier to understand, and might
also prevent some deopts from properties being changes on the socket
itself.

Also moves the Timeout constructor into an internal module.

PR-URL: #17704
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Author: Fishrock123

lib/internal/timers.js

@@ -0,0 +1,107 @@
+'use strict';
+
+const async_wrap = process.binding('async_wrap');
+// Two arrays that share state between C++ and JS.
+const { async_hook_fields, async_id_fields } = async_wrap;
+const {
+  getDefaultTriggerAsyncId,
+  // The needed emit*() functions.
+  emitInit
+} = require('internal/async_hooks');
+// Grab the constants necessary for working with internal arrays.
+const { kInit, kAsyncIdCounter } = async_wrap.constants;
+// Symbols for storing async id state.
+const async_id_symbol = Symbol('asyncId');
+const trigger_async_id_symbol = Symbol('triggerId');
+
+const errors = require('internal/errors');
+
+// Timeout values > TIMEOUT_MAX are set to 1.
+const TIMEOUT_MAX = 2 ** 31 - 1;
+
+module.exports = {
+  TIMEOUT_MAX,
+  kTimeout: Symbol('timeout'), // For hiding Timeouts on other internals.
+  async_id_symbol,
+  trigger_async_id_symbol,
+  Timeout,
+  setUnrefTimeout,
+};
+
+// Timer constructor function.
+// The entire prototype is defined in lib/timers.js
+function Timeout(callback, after, args, isRepeat) {
+  after *= 1; // coalesce to number or NaN
+  if (!(after >= 1 && after <= TIMEOUT_MAX)) {
+    if (after > TIMEOUT_MAX) {
+      process.emitWarning(`${after} does not fit into` +
+                          ' a 32-bit signed integer.' +
+                          '\nTimeout duration was set to 1.',
+                          'TimeoutOverflowWarning');
+    }
+    after = 1; // schedule on next tick, follows browser behavior
+  }
+
+  this._called = false;
+  this._idleTimeout = after;
+  this._idlePrev = this;
+  this._idleNext = this;
+  this._idleStart = null;
+  // this must be set to null first to avoid function tracking
+  // on the hidden class, revisit in V8 versions after 6.2
+  this._onTimeout = null;
+  this._onTimeout = callback;
+  this._timerArgs = args;
+  this._repeat = isRepeat ? after : null;
+  this._destroyed = false;
+
+  this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
+  this[trigger_async_id_symbol] = getDefaultTriggerAsyncId();
+  if (async_hook_fields[kInit] > 0) {
+    emitInit(this[async_id_symbol],
+             'Timeout',
+             this[trigger_async_id_symbol],
+             this);
+  }
+}
+
+var timers;
+function getTimers() {
+  if (timers === undefined) {
+    timers = require('timers');
+  }
+  return timers;
+}
+
+function setUnrefTimeout(callback, after, arg1, arg2, arg3) {
+  // Type checking identical to setTimeout()
+  if (typeof callback !== 'function') {
+    throw new errors.TypeError('ERR_INVALID_CALLBACK');
+  }
+
+  let i, args;
+  switch (arguments.length) {
+    // fast cases
+    case 1:
+    case 2:
+      break;
+    case 3:
+      args = [arg1];
+      break;
+    case 4:
+      args = [arg1, arg2];
+      break;
+    default:
+      args = [arg1, arg2, arg3];
+      for (i = 5; i < arguments.length; i++) {
+        // extend array dynamically, makes .apply run much faster in v6.0.0
+        args[i - 2] = arguments[i];
+      }
+      break;
+  }
+
+  const timer = new Timeout(callback, after, args, false);
+  getTimers()._unrefActive(timer);
+
+  return timer;
+}

lib/net.js

@@ -57,6 +57,8 @@ var cluster = null;
 const errnoException = util._errnoException;
 const exceptionWithHostPort = util._exceptionWithHostPort;
 
+const { kTimeout, TIMEOUT_MAX, setUnrefTimeout } = require('internal/timers');
+
 function noop() {}
 
 function createHandle(fd, is_server) {
@@ -201,6 +203,7 @@ function Socket(options) {
   this._parent = null;
   this._host = null;
   this[kLastWriteQueueSize] = 0;
+  this[kTimeout] = null;
 
   if (typeof options === 'number')
     options = { fd: options }; // Legacy interface.
@@ -272,9 +275,12 @@ function Socket(options) {
 }
 util.inherits(Socket, stream.Duplex);
 
+// Refresh existing timeouts.
 Socket.prototype._unrefTimer = function _unrefTimer() {
-  for (var s = this; s !== null; s = s._parent)
-    timers._unrefActive(s);
+  for (var s = this; s !== null; s = s._parent) {
+    if (s[kTimeout])
+      timers._unrefActive(s[kTimeout]);
+  }
 };
 
 
@@ -387,14 +393,36 @@ Socket.prototype.read = function(n) {
 };
 
 Socket.prototype.setTimeout = function(msecs, callback) {
+  // Type checking identical to timers.enroll()
+  if (typeof msecs !== 'number') {
+    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'msecs',
+                               'number', msecs);
+  }
+
+  if (msecs < 0 || !isFinite(msecs)) {
+    throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE', 'msecs',
+                                'a non-negative finite number', msecs);
+  }
+
+  // Ensure that msecs fits into signed int32
+  if (msecs > TIMEOUT_MAX) {
+    process.emitWarning(`${msecs} does not fit into a 32-bit signed integer.` +
+                        `\nTimer duration was truncated to ${TIMEOUT_MAX}.`,
+                        'TimeoutOverflowWarning');
+    msecs = TIMEOUT_MAX;
+  }
+
+  // Attempt to clear an existing timer lear in both cases -
+  //  even if it will be rescheduled we don't want to leak an existing timer.
+  clearTimeout(this[kTimeout]);
+
   if (msecs === 0) {
-    timers.unenroll(this);
     if (callback) {
       this.removeListener('timeout', callback);
     }
   } else {
-    timers.enroll(this, msecs);
-    timers._unrefActive(this);
+    this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs);
+
     if (callback) {
       this.once('timeout', callback);
     }
@@ -551,8 +579,9 @@ Socket.prototype._destroy = function(exception, cb) {
 
   this.readable = this.writable = false;
 
-  for (var s = this; s !== null; s = s._parent)
-    timers.unenroll(s);
+  for (var s = this; s !== null; s = s._parent) {
+    clearTimeout(s[kTimeout]);
+  }
 
   debug('close');
   if (this._handle) {

lib/timers.js

@@ -24,6 +24,7 @@
 const async_wrap = process.binding('async_wrap');
 const TimerWrap = process.binding('timer_wrap').Timer;
 const L = require('internal/linkedlist');
+const timerInternals = require('internal/timers');
 const internalUtil = require('internal/util');
 const { createPromise, promiseResolve } = process.binding('util');
 const assert = require('assert');
@@ -44,8 +45,8 @@ const {
 // Grab the constants necessary for working with internal arrays.
 const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
 // Symbols for storing async id state.
-const async_id_symbol = Symbol('asyncId');
-const trigger_async_id_symbol = Symbol('triggerAsyncId');
+const async_id_symbol = timerInternals.async_id_symbol;
+const trigger_async_id_symbol = timerInternals.trigger_async_id_symbol;
 
 /* This is an Uint32Array for easier sharing with C++ land. */
 const scheduledImmediateCount = process._scheduledImmediateCount;
@@ -55,7 +56,10 @@ const activateImmediateCheck = process._activateImmediateCheck;
 delete process._activateImmediateCheck;
 
 // Timeout values > TIMEOUT_MAX are set to 1.
-const TIMEOUT_MAX = 2 ** 31 - 1;
+const TIMEOUT_MAX = timerInternals.TIMEOUT_MAX;
+
+// The Timeout class
+const Timeout = timerInternals.Timeout;
 
 
 // HOW and WHY the timers implementation works the way it does.
@@ -446,12 +450,17 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
       break;
   }
 
-  return new Timeout(callback, after, args, false);
+  const timeout = new Timeout(callback, after, args, false);
+  active(timeout);
+
+  return timeout;
 }
 
 setTimeout[internalUtil.promisify.custom] = function(after, value) {
   const promise = createPromise();
-  new Timeout(promise, after, [value], false);
+  const timeout = new Timeout(promise, after, [value], false);
+  active(timeout);
+
   return promise;
 };
 
@@ -523,7 +532,10 @@ exports.setInterval = function(callback, repeat, arg1, arg2, arg3) {
       break;
   }
 
-  return new Timeout(callback, repeat, args, true);
+  const timeout = new Timeout(callback, repeat, args, true);
+  active(timeout);
+
+  return timeout;
 };
 
 exports.clearInterval = function(timer) {
@@ -534,44 +546,6 @@ exports.clearInterval = function(timer) {
 };
 
 
-function Timeout(callback, after, args, isRepeat) {
-  after *= 1; // coalesce to number or NaN
-  if (!(after >= 1 && after <= TIMEOUT_MAX)) {
-    if (after > TIMEOUT_MAX) {
-      process.emitWarning(`${after} does not fit into` +
-                          ' a 32-bit signed integer.' +
-                          '\nTimeout duration was set to 1.',
-                          'TimeoutOverflowWarning');
-    }
-    after = 1; // schedule on next tick, follows browser behavior
-  }
-
-  this._called = false;
-  this._idleTimeout = after;
-  this._idlePrev = this;
-  this._idleNext = this;
-  this._idleStart = null;
-  // this must be set to null first to avoid function tracking
-  // on the hidden class, revisit in V8 versions after 6.2
-  this._onTimeout = null;
-  this._onTimeout = callback;
-  this._timerArgs = args;
-  this._repeat = isRepeat ? after : null;
-  this._destroyed = false;
-
-  this[async_id_symbol] = ++async_id_fields[kAsyncIdCounter];
-  this[trigger_async_id_symbol] = getDefaultTriggerAsyncId();
-  if (async_hook_fields[kInit] > 0) {
-    emitInit(this[async_id_symbol],
-             'Timeout',
-             this[trigger_async_id_symbol],
-             this);
-  }
-
-  active(this);
-}
-
-
 function unrefdHandle() {
   // Don't attempt to call the callback if it is not a function.
   if (typeof this.owner._onTimeout === 'function') {

node.gyp

@@ -123,6 +123,7 @@
       'lib/internal/repl/await.js',
       'lib/internal/socket_list.js',
       'lib/internal/test/unicode.js',
+      'lib/internal/timers.js',
       'lib/internal/tls.js',
       'lib/internal/trace_events_async_hooks.js',
       'lib/internal/url.js',

test/parallel/test-http-client-timeout-on-connect.js

@@ -1,7 +1,11 @@
+// Flags: --expose-internals
+
 'use strict';
+
 const common = require('../common');
 const assert = require('assert');
 const http = require('http');
+const { kTimeout } = require('internal/timers');
 
 const server = http.createServer((req, res) => {
   // This space is intentionally left blank.
@@ -13,9 +17,9 @@ server.listen(0, common.localhostIPv4, common.mustCall(() => {
 
   req.setTimeout(1);
   req.on('socket', common.mustCall((socket) => {
-    assert.strictEqual(socket._idleTimeout, undefined);
+    assert.strictEqual(socket[kTimeout], null);
     socket.on('connect', common.mustCall(() => {
-      assert.strictEqual(socket._idleTimeout, 1);
+      assert.strictEqual(socket[kTimeout]._idleTimeout, 1);
     }));
   }));
   req.on('timeout', common.mustCall(() => req.abort()));

test/parallel/test-net-socket-timeout.js

@@ -36,13 +36,13 @@ const validDelays = [0, 0.001, 1, 1e6];
 for (let i = 0; i < nonNumericDelays.length; i++) {
   assert.throws(function() {
     s.setTimeout(nonNumericDelays[i], () => {});
-  }, TypeError);
+  }, TypeError, nonNumericDelays[i]);
 }
 
 for (let i = 0; i < badRangeDelays.length; i++) {
   assert.throws(function() {
     s.setTimeout(badRangeDelays[i], () => {});
-  }, RangeError);
+  }, RangeError, badRangeDelays[i]);
 }
 
 for (let i = 0; i < validDelays.length; i++) {

test/parallel/test-tls-wrap-timeout.js

@@ -1,5 +1,8 @@
+// Flags: --expose_internals
+
 'use strict';
 const common = require('../common');
+const { kTimeout, TIMEOUT_MAX } = require('internal/timers');
 
 if (!common.hasCrypto)
   common.skip('missing crypto');
@@ -30,13 +33,13 @@ let lastIdleStart;
 
 server.listen(0, () => {
   socket = net.connect(server.address().port, function() {
-    const s = socket.setTimeout(Number.MAX_VALUE, function() {
+    const s = socket.setTimeout(TIMEOUT_MAX, function() {
       throw new Error('timeout');
     });
     assert.ok(s instanceof net.Socket);
 
-    assert.notStrictEqual(socket._idleTimeout, -1);
-    lastIdleStart = socket._idleStart;
+    assert.notStrictEqual(socket[kTimeout]._idleTimeout, -1);
+    lastIdleStart = socket[kTimeout]._idleStart;
 
     const tsocket = tls.connect({
       socket: socket,
@@ -47,6 +50,6 @@ server.listen(0, () => {
 });
 
 process.on('exit', () => {
-  assert.strictEqual(socket._idleTimeout, -1);
-  assert(lastIdleStart < socket._idleStart);
+  assert.strictEqual(socket[kTimeout]._idleTimeout, -1);
+  assert(lastIdleStart < socket[kTimeout]._idleStart);
 });