http2: make request() invalid-state errors async

PR-URL: #63414
Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

doc/api/http2.md

@@ -1113,10 +1113,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

@@ -343,8 +343,6 @@ const SESSION_FLAGS_PENDING = 0x0;
 const SESSION_FLAGS_READY = 0x1;
 const SESSION_FLAGS_CLOSED = 0x2;
 const SESSION_FLAGS_DESTROYED = 0x4;
-const SESSION_FLAGS_CLOSE_QUEUED = 0x8;
-const SESSION_FLAGS_CLOSE_EMITTED = 0x10;
 
 // Top level to avoid creating a closure
 function emit(self, ...args) {
@@ -840,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
@@ -1158,26 +1160,11 @@ function setupHandle(socket, type, options) {
 // Emits an error event followed by a close event if err is truthy. Used
 // by Http2Session.prototype.destroy()
 function emitClose(self, error) {
-  const state = self[kState];
-  if (state.flags & SESSION_FLAGS_CLOSE_EMITTED)
-    return;
-
-  state.flags |= SESSION_FLAGS_CLOSE_EMITTED;
-
   if (error)
     self.emit('error', error);
   self.emit('close');
 }
 
-function emitCloseNextTick(self, error) {
-  const state = self[kState];
-  if (state.flags & (SESSION_FLAGS_CLOSE_QUEUED | SESSION_FLAGS_CLOSE_EMITTED))
-    return;
-
-  state.flags |= SESSION_FLAGS_CLOSE_QUEUED;
-  process.nextTick(emitClose, self, error);
-}
-
 function cleanupSession(session) {
   const socket = session[kSocket];
   const handle = session[kHandle];
@@ -1226,7 +1213,7 @@ function finishSessionClose(session, error) {
       }
     });
   } else {
-    emitCloseNextTick(session, error);
+    process.nextTick(emitClose, session, error);
   }
 }
 
@@ -1243,13 +1230,6 @@ function closeSession(session, code, error) {
 
   const socket = session[kSocket];
   const handle = session[kHandle];
-  const socketDestroyed = socket?.destroyed === true;
-
-  // If the transport has already closed, queue the session close event before
-  // stream callbacks are scheduled so clients can invalidate cached sessions
-  // before associated streams finish closing.
-  if (socketDestroyed)
-    emitCloseNextTick(session, error);
 
   // Destroy any pending and open streams
   if (state.pendingStreams.size > 0 || state.streams.size > 0) {
@@ -1261,7 +1241,7 @@ function closeSession(session, code, error) {
   // Destroy the handle if it exists at this point.
   if (handle !== undefined) {
     handle.ondone = finishSessionClose.bind(null, session, error);
-    handle.destroy(code, socketDestroyed);
+    handle.destroy(code, socket.destroyed);
   } else {
     finishSessionClose(session, error);
   }
@@ -1832,11 +1812,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]();
 
@@ -1922,19 +1906,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 { listenerCount } = 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 { listenerCount } = 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

@@ -32,23 +32,28 @@ server.on('stream', common.mustCall((stream, headers) => {
 server.listen(0, common.mustCall(() => {
   const session = http2.connect(`http://localhost:${server.address().port}`);
   let cachedSession = session;
-  const events = [];
 
-  session.on('error', common.mustNotCall());
+  session.on('error', () => {});
   session.on('close', common.mustCall(() => {
-    events.push('session-close');
     cachedSession = undefined;
     server.close();
   }));
 
   const req = session.request({ ':path': '/close' });
   req.on('response', common.mustCall());
+  req.on('error', () => {});
   req.on('close', common.mustCall(() => {
-    events.push('stream-close');
-    assert.strictEqual(session.closed, true);
-    assert.strictEqual(session.destroyed, true);
-    assert.strictEqual(cachedSession, undefined);
-    assert.deepStrictEqual(events, ['session-close', 'stream-close']);
+    // 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();
 }));