http2: use actual Timeout instances

This makes `Http2Stream`s and `Http2Session`s use actual Timeout objects in a [kTimeout] symbol property, rather than making the stream/session itself a timer and appending properties to it directly. 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>
nodejs · Dec 20, 2017 · 93eb68e · 93eb68e
1 parent 593941a
commit 93eb68e
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 29 deletions.
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -52,10 +52,12 @@ const {
 } = require('internal/http2/util');
 
 const {
-  _unrefActive,
-  enroll,
-  unenroll
-} = require('timers');
+  kTimeout,
+  setUnrefTimeout,
+  validateTimerDuration
+} = require('internal/timers');
+
+const { _unrefActive } = require('timers');
 
 const { ShutdownWrap, WriteWrap } = process.binding('stream_wrap');
 const { constants } = binding;
@@ -280,8 +282,8 @@ function onStreamClose(code, hasData) {
         ` [has data? ${hasData}]`);
 
   if (!stream.closed) {
-    // Unenroll from timeouts
-    unenroll(stream);
+    // Clear timeout and remove timeout listeners
+    stream.setTimeout(0);
     stream.removeAllListeners('timeout');
 
     // Set the state flags
@@ -788,6 +790,7 @@ class Http2Session extends EventEmitter {
     this[kType] = type;
     this[kProxySocket] = null;
     this[kSocket] = socket;
+    this[kTimeout] = null;
 
     // Do not use nagle's algorithm
     if (typeof socket.setNoDelay === 'function')
@@ -828,7 +831,7 @@ class Http2Session extends EventEmitter {
   [kUpdateTimer]() {
     if (this.destroyed)
       return;
-    _unrefActive(this);
+    if (this[kTimeout]) _unrefActive(this[kTimeout]);
   }
 
   // Sets the id of the next stream to be created by this Http2Session.
@@ -1019,7 +1022,7 @@ class Http2Session extends EventEmitter {
     state.flags |= SESSION_FLAGS_DESTROYED;
 
     // Clear timeout and remove timeout listeners
-    unenroll(this);
+    this.setTimeout(0);
     this.removeAllListeners('timeout');
 
     // Destroy any pending and open streams
@@ -1322,6 +1325,8 @@ class Http2Stream extends Duplex {
     this[kSession] = session;
     session[kState].pendingStreams.add(this);
 
+    this[kTimeout] = null;
+
     this[kState] = {
       flags: STREAM_FLAGS_PENDING,
       rstCode: NGHTTP2_NO_ERROR,
@@ -1336,9 +1341,10 @@ class Http2Stream extends Duplex {
   [kUpdateTimer]() {
     if (this.destroyed)
       return;
-    _unrefActive(this);
-    if (this[kSession])
-      _unrefActive(this[kSession]);
+    if (this[kTimeout])
+      _unrefActive([kTimeout]);
+    if (this[kSession] && this[kSession][kTimeout])
+      _unrefActive(this[kSession][kTimeout]);
   }
 
   [kInit](id, handle) {
@@ -1560,7 +1566,7 @@ class Http2Stream extends Duplex {
 
   // Close initiates closing the Http2Stream instance by sending an RST_STREAM
   // frame to the connected peer. The readable and writable sides of the
-  // Http2Stream duplex are closed and the timeout timer is unenrolled. If
+  // Http2Stream duplex are closed and the timeout timer is cleared. If
   // a callback is passed, it is registered to listen for the 'close' event.
   //
   // If the handle and stream ID have not been assigned yet, the close
@@ -1577,8 +1583,8 @@ class Http2Stream extends Duplex {
     if (code < 0 || code > kMaxInt)
       throw new errors.RangeError('ERR_OUT_OF_RANGE', 'code');
 
-    // Unenroll the timeout.
-    unenroll(this);
+    // Clear timeout and remove timeout listeners
+    this.setTimeout(0);
     this.removeAllListeners('timeout');
 
     // Close the writable
@@ -1637,8 +1643,10 @@ class Http2Stream extends Duplex {
       handle.destroy();
       session[kState].streams.delete(id);
     } else {
-      unenroll(this);
+      // Clear timeout and remove timeout listeners
+      this.setTimeout(0);
       this.removeAllListeners('timeout');
+
       state.flags |= STREAM_FLAGS_CLOSED;
       abort(this);
       this.end();
@@ -2216,21 +2224,24 @@ const setTimeout = {
   value: function(msecs, callback) {
     if (this.destroyed)
       return;
-    if (typeof msecs !== 'number') {
-      throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
-                                 'msecs',
-                                 'number');
-    }
+
+    // Type checking identical to timers.enroll()
+    msecs = validateTimerDuration(msecs);
+
+    // 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) {
-      unenroll(this);
       if (callback !== undefined) {
         if (typeof callback !== 'function')
           throw new errors.TypeError('ERR_INVALID_CALLBACK');
         this.removeListener('timeout', callback);
       }
     } else {
-      enroll(this, msecs);
-      this[kUpdateTimer]();
+      this[kTimeout] = setUnrefTimeout(this._onTimeout.bind(this), msecs);
+      if (this[kSession]) this[kSession][kUpdateTimer]();
+
       if (callback !== undefined) {
         if (typeof callback !== 'function')
           throw new errors.TypeError('ERR_INVALID_CALLBACK');

diff --git a/test/parallel/test-http2-compat-socket.js b/test/parallel/test-http2-compat-socket.js
@@ -1,3 +1,5 @@
+// Flags: --expose_internals
+
 'use strict';
 
 const common = require('../common');
@@ -7,6 +9,8 @@ const assert = require('assert');
 const h2 = require('http2');
 const net = require('net');
 
+const { kTimeout } = require('internal/timers');
+
 // Tests behavior of the proxied socket in Http2ServerRequest
 // & Http2ServerResponse - this proxy socket should mimic the
 // behavior of http1 but against the http2 api & model
@@ -31,7 +35,7 @@ server.on('request', common.mustCall(function(request, response) {
   assert.strictEqual(request.socket.destroyed, false);
 
   request.socket.setTimeout(987);
-  assert.strictEqual(request.stream.session._idleTimeout, 987);
+  assert.strictEqual(request.stream.session[kTimeout]._idleTimeout, 987);
   request.socket.setTimeout(0);
 
   common.expectsError(() => request.socket.read(), errMsg);

diff --git a/test/parallel/test-http2-socket-proxy.js b/test/parallel/test-http2-socket-proxy.js
@@ -1,3 +1,5 @@
+// Flags: --expose_internals
+
 'use strict';
 
 const common = require('../common');
@@ -7,6 +9,8 @@ const assert = require('assert');
 const h2 = require('http2');
 const net = require('net');
 
+const { kTimeout } = require('internal/timers');
+
 // Tests behavior of the proxied socket on Http2Session
 
 const errMsg = {
@@ -29,7 +33,7 @@ server.on('stream', common.mustCall(function(stream, headers) {
   assert.strictEqual(typeof socket.address(), 'object');
 
   socket.setTimeout(987);
-  assert.strictEqual(session._idleTimeout, 987);
+  assert.strictEqual(session[kTimeout]._idleTimeout, 987);
 
   common.expectsError(() => socket.destroy, errMsg);
   common.expectsError(() => socket.emit, errMsg);
@@ -59,9 +63,6 @@ server.on('stream', common.mustCall(function(stream, headers) {
 
   stream.end();
 
-  socket.setTimeout = undefined;
-  assert.strictEqual(session.setTimeout, undefined);
-
   stream.session.on('close', common.mustCall(() => {
     assert.strictEqual(session.socket, undefined);
   }));

diff --git a/test/parallel/test-http2-timeouts.js b/test/parallel/test-http2-timeouts.js
@@ -20,7 +20,8 @@ server.on('stream', common.mustCall((stream) => {
     {
       code: 'ERR_INVALID_ARG_TYPE',
       type: TypeError,
-      message: 'The "msecs" argument must be of type number'
+      message:
+        'The "msecs" argument must be of type number. Received type string'
     }
   );
   common.expectsError(