http2: use session not socket timeout, tests

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);
-  process.nextTick(() => this[kOwner].emit('error', error));
+  const owner = this[kOwner];
+  _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();
+    }
+  }
+}));