http2: emit session close before stream close

Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #63414
Fixes: #63412
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Author: mcollina

doc/api/http2.md

@@ -1109,10 +1109,12 @@ creates and returns an `Http2Stream` instance that can be used to send an
 HTTP/2 request to the connected server.
 
 When a `ClientHttp2Session` is first created, the socket may not yet be
-connected. if `clienthttp2session.request()` is called during this time, the
+connected. If `clienthttp2session.request()` is called during this time, the
 actual request will be deferred until the socket is ready to go.
-If the `session` is closed before the actual request be executed, an
-`ERR_HTTP2_GOAWAY_SESSION` is thrown.
+
+If the session becomes unavailable before the request can be created, the
+returned stream will emit `ERR_HTTP2_GOAWAY_SESSION` or
+`ERR_HTTP2_INVALID_SESSION` asynchronously.
 
 This method is only available if `http2session.type` is equal to
 `http2.constants.NGHTTP2_SESSION_CLIENT`.

lib/internal/http2/core.js

@@ -838,6 +838,10 @@ function requestOnConnect(headersList, options) {
   }
 }
 
+function requestOnError(error) {
+  this.destroy(error);
+}
+
 // Validates that priority options are correct, specifically:
 // 1. options.weight must be a number
 // 2. options.parent must be a positive number
@@ -1153,7 +1157,7 @@ function setupHandle(socket, type, options) {
   process.nextTick(emit, this, 'connect', this, socket);
 }
 
-// Emits a close event followed by an error event if err is truthy. Used
+// Emits an error event followed by a close event if err is truthy. Used
 // by Http2Session.prototype.destroy()
 function emitClose(self, error) {
   if (error)
@@ -1220,17 +1224,16 @@ function closeSession(session, code, error) {
   session.setTimeout(0);
   session.removeAllListeners('timeout');
 
+  const socket = session[kSocket];
+  const handle = session[kHandle];
+
   // 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 if it exists at this point.
   if (handle !== undefined) {
     handle.ondone = finishSessionClose.bind(null, session, error);
@@ -1800,11 +1803,15 @@ class ClientHttp2Session extends Http2Session {
   request(headersParam, options) {
     debugSessionObj(this, 'initiating request');
 
-    if (this.destroyed)
-      throw new ERR_HTTP2_INVALID_SESSION();
-
-    if (this.closed)
-      throw new ERR_HTTP2_GOAWAY_SESSION();
+    // Keep argument validation synchronous, but defer session-state failures
+    // to the returned stream so request retries from stream callbacks do not
+    // throw before session lifecycle handlers run.
+    let requestError;
+    if (this.destroyed) {
+      requestError = new ERR_HTTP2_INVALID_SESSION();
+    } else if (this.closed) {
+      requestError = new ERR_HTTP2_GOAWAY_SESSION();
+    }
 
     this[kUpdateTimer]();
 
@@ -1890,19 +1897,24 @@ class ClientHttp2Session extends Http2Session {
       }
     }
 
-    const onConnect = reqAsync.bind(requestOnConnect.bind(stream, headersList, options));
-    if (this.connecting) {
-      if (this[kPendingRequestCalls] !== null) {
-        this[kPendingRequestCalls].push(onConnect);
+    if (requestError) {
+      process.nextTick(reqAsync.bind(requestOnError.bind(stream, requestError)));
+    } else {
+      const onConnect = reqAsync.bind(
+        requestOnConnect.bind(stream, headersList, options));
+      if (this.connecting) {
+        if (this[kPendingRequestCalls] !== null) {
+          this[kPendingRequestCalls].push(onConnect);
+        } else {
+          this[kPendingRequestCalls] = [onConnect];
+          this.once('connect', () => {
+            this[kPendingRequestCalls].forEach((f) => f());
+            this[kPendingRequestCalls] = null;
+          });
+        }
       } else {
-        this[kPendingRequestCalls] = [onConnect];
-        this.once('connect', () => {
-          this[kPendingRequestCalls].forEach((f) => f());
-          this[kPendingRequestCalls] = null;
-        });
+        onConnect();
       }
-    } else {
-      onConnect();
     }
 
     if (onClientStreamCreatedChannel.hasSubscribers) {

test/parallel/test-http2-client-destroy.js

@@ -81,7 +81,19 @@ const { getEventListeners } = require('events');
     assert.throws(() => client.ping(), sessionError);
     assert.throws(() => client.settings({}), sessionError);
     assert.throws(() => client.goaway(), sessionError);
-    assert.throws(() => client.request(), sessionError);
+
+    const pendingReq = client.request();
+    pendingReq.on('response', common.mustNotCall());
+    pendingReq.on('error', common.expectsError(sessionError));
+    pendingReq.on('close', common.mustCall());
+
+    client.on('close', common.mustCall(() => {
+      const postCloseReq = client.request();
+      postCloseReq.on('response', common.mustNotCall());
+      postCloseReq.on('error', common.expectsError(sessionError));
+      postCloseReq.on('close', common.mustCall());
+    }));
+
     client.close();  // Should be a non-op at this point
 
     // Wait for setImmediate call from destroy() to complete
@@ -92,7 +104,6 @@ const { getEventListeners } = require('events');
       assert.throws(() => client.ping(), sessionError);
       assert.throws(() => client.settings({}), sessionError);
       assert.throws(() => client.goaway(), sessionError);
-      assert.throws(() => client.request(), sessionError);
       client.close();  // Should be a non-op at this point
     });
 

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

@@ -0,0 +1,59 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const http2 = require('http2');
+
+const server = http2.createServer();
+let serverSocket;
+
+server.on('connection', common.mustCall((socket) => {
+  serverSocket = socket;
+  socket.on('error', () => {});
+}));
+
+server.on('sessionError', () => {});
+server.on('stream', common.mustCall((stream, headers) => {
+  if (headers[':path'] === '/close') {
+    stream.respond({ ':status': 200 });
+    stream.write('partial', common.mustCall(() => {
+      setImmediate(() => serverSocket.destroy());
+    }));
+    return;
+  }
+
+  stream.respond({ ':status': 200 });
+  stream.end('ok');
+}));
+
+server.listen(0, common.mustCall(() => {
+  const session = http2.connect(`http://localhost:${server.address().port}`);
+  let cachedSession = session;
+
+  session.on('error', () => {});
+  session.on('close', common.mustCall(() => {
+    cachedSession = undefined;
+    server.close();
+  }));
+
+  const req = session.request({ ':path': '/close' });
+  req.on('response', common.mustCall());
+  req.on('error', () => {});
+  req.on('close', common.mustCall(() => {
+    // This must not throw synchronously even though the session is no longer
+    // usable. Depending on teardown timing, the returned stream may report a
+    // closed session before the destroy state is fully observable here.
+    const req2 = session.request({ ':path': '/again' });
+
+    req2.on('error', common.mustCall((err) => {
+      assert.ok(
+        err.code === 'ERR_HTTP2_INVALID_SESSION' ||
+        err.code === 'ERR_HTTP2_GOAWAY_SESSION');
+      assert.strictEqual(cachedSession, undefined);
+    }));
+  }));
+  req.resume();
+}));