http2: ensure session.request() does not throw synchronously in stream close callbacks

Author: DavidUmunna

lib/internal/http2/core.js

@@ -1297,30 +1297,24 @@ function closeSession(session, code, error) {
   session.setTimeout(0);
   session.removeAllListeners('timeout');
 
+  // Destroy any pending and open streams
+  if (state.pendingStreams.size > 0 || state.streams.size > 0) {
+    const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
+    state.pendingStreams.forEach((stream) => stream.destroy(cancel));
+    state.streams.forEach((stream) => stream.destroy(error));
+  }
+
   // Disassociate from the socket and server.
   const socket = session[kSocket];
   const handle = session[kHandle];
 
-  // Destroy the handle *before* destroying streams. When the socket is already
-  // closed the handle's ondone callback (finishSessionClose) is invoked
-  // synchronously from handle.destroy(), which schedules the session 'close'
-  // event via process.nextTick. Destroying the streams afterwards ensures their
-  // 'close' events are queued on the nextTick queue *after* the session 'close'
-  // event, so user code receives the session-level signal before any stream
-  // callbacks observe session.closed/session.destroyed as true.
+  // Destroy the handle if it exists at this point.
   if (handle !== undefined) {
     handle.ondone = finishSessionClose.bind(null, session, error);
     handle.destroy(code, socket.destroyed);
   } else {
     finishSessionClose(session, error);
   }
-
-  // Destroy any pending and open streams
-  if (state.pendingStreams.size > 0 || state.streams.size > 0) {
-    const cancel = new ERR_HTTP2_STREAM_CANCEL(error);
-    state.pendingStreams.forEach((stream) => stream.destroy(cancel));
-    state.streams.forEach((stream) => stream.destroy(error));
-  }
 }
 
 // Upon creation, the Http2Session takes ownership of the socket. The session

test/parallel/test-http2-session-close-before-stream-close.js

@@ -1,9 +1,10 @@
 'use strict';
 
 // Regression test: when the server abruptly destroys the underlying socket,
-// the client session 'close' event must fire before any stream 'close'
-// callback can observe session.closed/session.destroyed as true.
-// See https://github.com/nodejs/node/issues/<issue>
+// session.request() must not throw synchronously inside a stream 'close'
+// callback. Instead it should return a stream that emits 'error' async with
+// ERR_HTTP2_INVALID_SESSION, giving session lifecycle handlers a chance to
+// clear their cached session reference before any retry occurs.
 
 const common = require('../common');
 if (!common.hasCrypto)
@@ -36,33 +37,34 @@ server.on('stream', (stream, headers) => {
 server.listen(0, common.mustCall(() => {
   const session = http2.connect(`http://localhost:${server.address().port}`);
 
-  let sessionCloseFired = false;
-
-  session.on('close', common.mustCall(() => {
-    sessionCloseFired = true;
-    server.close();
-  }));
-
-  // We accept that an error may or may not fire, but it must fire before
-  // any stream close callback sees session.destroyed.
+  session.on('close', common.mustCall(() => server.close()));
   session.on('error', () => {});
 
   const req = session.request({ ':path': '/close' });
   req.resume();
   req.on('response', () => {});
+  req.on('error', () => {}); // socket may emit ECONNRESET on abrupt close
 
-  // The stream 'close' event must not observe the session as destroyed
-  // before the session 'close' event has fired.
   req.on('close', common.mustCall(() => {
-    // If the session is already destroyed, the session 'close' event must
-    // have already been emitted (sessionCloseFired === true).
-    if (session.destroyed) {
-      assert.strictEqual(
-        sessionCloseFired,
-        true,
-        'session "close" event must fire before stream "close" callback ' +
-        'observes session.destroyed === true',
-      );
+    if (!session.destroyed) return;
+
+    // session.request() must NOT throw synchronously even though the session
+    // is already destroyed. It should return a stream that errors async.
+    let threw = false;
+    let req2;
+    try {
+      req2 = session.request({ ':path': '/again' });
+    } catch {
+      threw = true;
     }
+
+    assert.strictEqual(threw, false,
+      'session.request() must not throw synchronously inside a stream close callback');
+
+    // The returned stream should asynchronously emit ERR_HTTP2_INVALID_SESSION
+    req2.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_HTTP2_INVALID_SESSION');
+    }));
+    req2.resume();
   }));
 }));