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: https://github.com/nodejs/node/pull/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>

1 files changed, 34 insertions, 23 deletions

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 7d8ec2e772..ff5e88f6c6 100644
--- 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');