http2: use session not socket timeout, tests

Change default timeout to be tracked on the session instead
of the socket, as nghttp2 manages the socket and we would
need to maintain two sets of timeouts for similar purpose.
Also fixes session setTimeout to work as it wasn't getting
_unrefActive correctly (was called on the handle).

Fixes: https://github.com/nodejs/node/issues/15158
PR-URL: https://github.com/nodejs/node/pull/15188
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

lib/internal/http2/core.js

@@ -150,8 +150,8 @@ function emit() {
 // event. If the stream is not new, emit the 'headers' event to pass
 // the block of headers on.
 function onSessionHeaders(id, cat, flags, headers) {
-  _unrefActive(this);
   const owner = this[kOwner];
+  _unrefActive(owner);
   debug(`[${sessionName(owner[kType])}] headers were received on ` +
         `stream ${id}: ${cat}`);
   const streams = owner[kState].streams;
@@ -265,7 +265,7 @@ function onSessionStreamClose(id, code) {
   const stream = owner[kState].streams.get(id);
   if (stream === undefined)
     return;
-  _unrefActive(this);
+  _unrefActive(owner);
   // Set the rst state for the stream
   abort(stream);
   const state = stream[kState];
@@ -287,14 +287,16 @@ function afterFDClose(err) {
 
 // Called when an error event needs to be triggered
 function onSessionError(error) {
-  _unrefActive(this);
+  const owner = this[kOwner];
-  process.nextTick(() => this[kOwner].emit('error', error));
+  _unrefActive(owner);
+  process.nextTick(() => owner.emit('error', error));
 }
 
 // Receives a chunk of data for a given stream and forwards it on
 // to the Http2Stream Duplex for processing.
 function onSessionRead(nread, buf, handle) {
-  const streams = this[kOwner][kState].streams;
+  const owner = this[kOwner];
+  const streams = owner[kState].streams;
   const id = handle.id;
   const stream = streams.get(id);
   // It should not be possible for the stream to not exist at this point.
@@ -303,7 +305,7 @@ function onSessionRead(nread, buf, handle) {
          'Internal HTTP/2 Failure. Stream does not exist. Please ' +
          'report this as a bug in Node.js');
   const state = stream[kState];
-  _unrefActive(this); // Reset the session timeout timer
+  _unrefActive(owner); // Reset the session timeout timer
   _unrefActive(stream); // Reset the stream timeout timer
 
   if (nread >= 0 && !stream.destroyed) {
@@ -322,7 +324,7 @@ function onSessionRead(nread, buf, handle) {
 function onSettings(ack) {
   const owner = this[kOwner];
   debug(`[${sessionName(owner[kType])}] new settings received`);
-  _unrefActive(this);
+  _unrefActive(owner);
   let event = 'remoteSettings';
   if (ack) {
     if (owner[kState].pendingAck > 0)
@@ -348,7 +350,7 @@ function onPriority(id, parent, weight, exclusive) {
   debug(`[${sessionName(owner[kType])}] priority advisement for stream ` +
         `${id}: \n  parent: ${parent},\n  weight: ${weight},\n` +
         `  exclusive: ${exclusive}`);
-  _unrefActive(this);
+  _unrefActive(owner);
   const streams = owner[kState].streams;
   const stream = streams.get(id);
   const emitter = stream === undefined ? owner : stream;
@@ -370,7 +372,7 @@ function onFrameError(id, type, code) {
   const owner = this[kOwner];
   debug(`[${sessionName(owner[kType])}] error sending frame type ` +
         `${type} on stream ${id}, code: ${code}`);
-  _unrefActive(this);
+  _unrefActive(owner);
   const streams = owner[kState].streams;
   const stream = streams.get(id);
   const emitter = stream !== undefined ? stream : owner;
@@ -975,7 +977,7 @@ class Http2Session extends EventEmitter {
     state.destroying = true;
 
     // Unenroll the timer
-    this.setTimeout(0);
+    this.setTimeout(0, sessionOnTimeout);
 
     // Shut down any still open streams
     const streams = state.streams;
@@ -2185,7 +2187,6 @@ function socketDestroy(error) {
   const type = this[kSession][kType];
   debug(`[${sessionName(type)}] socket destroy called`);
   delete this[kServer];
-  this.setTimeout(0);
   // destroy the session first so that it will stop trying to
   // send data while we close the socket.
   this[kSession].destroy();
@@ -2247,31 +2248,6 @@ function socketOnError(error) {
   this.destroy(error);
 }
 
-// When the socket times out on the server, attempt a graceful shutdown
-// of the session.
-function socketOnTimeout() {
-  debug('socket timeout');
-  process.nextTick(() => {
-    const server = this[kServer];
-    const session = this[kSession];
-    // If server or session are undefined, or session.destroyed is true
-    // then we're already in the process of shutting down, do nothing.
-    if (server === undefined || session === undefined)
-      return;
-    const state = session[kState];
-    if (state.destroyed || state.destroying)
-      return;
-    if (!server.emit('timeout', session, this)) {
-      session.shutdown(
-        {
-          graceful: true,
-          errorCode: NGHTTP2_NO_ERROR
-        },
-        this.destroy.bind(this));
-    }
-  });
-}
-
 // Handles the on('stream') event for a session and forwards
 // it on to the server object.
 function sessionOnStream(stream, headers, flags, rawHeaders) {
@@ -2289,15 +2265,34 @@ function sessionOnSocketError(error, socket) {
     this[kServer].emit('socketError', error, socket, this);
 }
 
+// When the session times out on the server, attempt a graceful shutdown
+function sessionOnTimeout() {
+  debug('session timeout');
+  process.nextTick(() => {
+    // if destroyed or destryoing, do nothing
+    if (this[kState].destroyed || this[kState].destroying)
+      return;
+    const server = this[kServer];
+    const socket = this[kSocket];
+    // If server or socket are undefined, then we're already in the process of
+    // shutting down, do nothing.
+    if (server === undefined || socket === undefined)
+      return;
+    if (!server.emit('timeout', this)) {
+      this.shutdown(
+        {
+          graceful: true,
+          errorCode: NGHTTP2_NO_ERROR
+        },
+        socket.destroy.bind(socket));
+    }
+  });
+}
+
 function connectionListener(socket) {
   debug('[server] received a connection');
   const options = this[kOptions] || {};
 
-  if (this.timeout) {
-    socket.setTimeout(this.timeout);
-    socket.on('timeout', socketOnTimeout);
-  }
-
   if (socket.alpnProtocol === false || socket.alpnProtocol === 'http/1.1') {
     if (options.allowHTTP1 === true) {
       // Fallback to HTTP/1.1
@@ -2325,6 +2320,11 @@ function connectionListener(socket) {
   session.on('priority', sessionOnPriority);
   session.on('socketError', sessionOnSocketError);
 
+  if (this.timeout) {
+    session.setTimeout(this.timeout);
+    session.on('timeout', sessionOnTimeout);
+  }
+
   socket[kServer] = this;
 
   process.nextTick(emit.bind(this, 'session', session));

test/parallel/test-http2-session-timeout.js

@@ -0,0 +1,41 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const h2 = require('http2');
+
+const serverTimeout = common.platformTimeout(200);
+const callTimeout = common.platformTimeout(10);
+
+const server = h2.createServer();
+server.timeout = serverTimeout;
+
+server.on('request', (req, res) => res.end());
+server.on('timeout', common.mustNotCall());
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url);
+  makeReq(40);
+
+  function makeReq(attempts) {
+    const request = client.request({
+      ':path': '/foobar',
+      ':method': 'GET',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`,
+    });
+    request.end();
+
+    if (attempts) {
+      setTimeout(() => makeReq(attempts - 1), callTimeout);
+    } else {
+      server.close();
+      client.destroy();
+    }
+  }
+}));