http2: guard against destroyed session, timeouts

Guard against destroyed session in timeouts and goaway event.

Improve timeout handling a bit.

PR-URL: https://github.com/nodejs/node/pull/15106
Fixes: https://github.com/nodejs/node/issues/14964
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

3 files changed, 38 insertions, 7 deletions

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index a8285fd9d9..479d0d36f6 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -377,6 +377,8 @@ function onFrameError(id, type, code) {
 function emitGoaway(state, code, lastStreamID, buf) {
   this.emit('goaway', code, lastStreamID, buf);
   // Tear down the session or destroy
+  if (state.destroying || state.destroyed)
+    return;
   if (!state.shuttingDown && !state.shutdown) {
     this.shutdown({}, this.destroy.bind(this));
   } else {
@@ -970,7 +972,7 @@ class Http2Session extends EventEmitter {
     state.destroying = true;
 
     // Unenroll the timer
-    unenroll(this);
+    this.setTimeout(0);
 
     // Shut down any still open streams
     const streams = state.streams;
@@ -1530,7 +1532,7 @@ class Http2Stream extends Duplex {
       session.removeListener('close', this[kState].closeHandler);
 
       // Unenroll the timer
-      unenroll(this);
+      this.setTimeout(0);
 
       setImmediate(finishStreamDestroy.bind(this, handle));
 
@@ -2180,7 +2182,7 @@ function socketDestroy(error) {
   const type = this[kSession][kType];
   debug(`[${sessionName(type)}] socket destroy called`);
   delete this[kServer];
-  this.removeListener('timeout', socketOnTimeout);
+  this.setTimeout(0);
   // destroy the session first so that it will stop trying to
   // send data while we close the socket.
   this[kSession].destroy();
@@ -2249,10 +2251,13 @@ function socketOnTimeout() {
   process.nextTick(() => {
     const server = this[kServer];
     const session = this[kSession];
-    // If server or session are undefined, then we're already in the process of
-    // shutting down, do nothing.
+    // 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(
         {
diff --git a/test/parallel/test-http2-client-destroy-goaway.js b/test/parallel/test-http2-client-destroy-goaway.js
new file mode 100644
index 0000000000..c393cfc070
--- /dev/null
+++ b/test/parallel/test-http2-client-destroy-goaway.js
@@ -0,0 +1,25 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const http2 = require('http2');
+
+const server = http2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  stream.on('error', common.mustCall());
+  stream.session.shutdown();
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+
+  client.on('goaway', common.mustCall(() => {
+    // We ought to be able to destroy the client in here without an error
+    server.close();
+    client.destroy();
+  }));
+
+  client.request();
+}));
diff --git a/test/parallel/test-http2-server-shutdown-before-respond.js b/test/parallel/test-http2-server-shutdown-before-respond.js
index 75e60f46e6..f870edc7be 100644
--- a/test/parallel/test-http2-server-shutdown-before-respond.js
+++ b/test/parallel/test-http2-server-shutdown-before-respond.js
@@ -26,9 +26,10 @@ server.on('listening', common.mustCall(() => {
 
   const client = h2.connect(`http://localhost:${server.address().port}`);
 
-  const req = client.request({ ':path': '/' });
+  client.on('goaway', common.mustCall());
+
+  const req = client.request();
 
   req.resume();
   req.on('end', common.mustCall(() => server.close()));
-  req.end();
 }));