diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 1c6edd65cae8f0..d6cb152f35b66e 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1812,14 +1812,23 @@ class ClientHttp2Session extends Http2Session { request(headersParam, options) { debugSessionObj(this, 'initiating request'); - // 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(); + // Session-state errors are deferred asynchronously rather than thrown + // synchronously. This prevents a race where stream 'close' callbacks + // observe session.destroyed/closed before the session 'close' event has + // fired: with a sync throw, code that retries inside a stream callback + // would crash before its session lifecycle handlers could clear the cached + // session. By returning a stream that emits 'error' on the next tick we + // match the behaviour of session.request() while connecting, where errors + // are also deferred, and give event-driven code a chance to handle both + // session and stream cleanup in any order. + if (this.destroyed || this.closed) { + // eslint-disable-next-line no-use-before-define + const stream = new ClientHttp2Stream(this, undefined, undefined, {}); + const err = this.destroyed + ? new ERR_HTTP2_INVALID_SESSION() + : new ERR_HTTP2_GOAWAY_SESSION(); + process.nextTick(() => stream.destroy(err)); + return stream; } this[kUpdateTimer](); diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index 7034f98abb6836..794f82554b57d3 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/test/parallel/test-http2-client-destroy.js @@ -81,19 +81,13 @@ const { listenerCount } = require('events'); assert.throws(() => client.ping(), sessionError); assert.throws(() => client.settings({}), sessionError); assert.throws(() => client.goaway(), 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()); - })); - + // request() now returns a stream that errors asynchronously when the + // session is destroyed, instead of throwing synchronously. + { + const reqAfterDestroy = client.request(); + reqAfterDestroy.on('response', common.mustNotCall()); + reqAfterDestroy.on('error', common.expectsError(sessionError)); + } client.close(); // Should be a non-op at this point // Wait for setImmediate call from destroy() to complete @@ -104,6 +98,11 @@ const { listenerCount } = require('events'); assert.throws(() => client.ping(), sessionError); assert.throws(() => client.settings({}), sessionError); assert.throws(() => client.goaway(), sessionError); + { + const reqAfterDestroy = client.request(); + reqAfterDestroy.on('response', common.mustNotCall()); + reqAfterDestroy.on('error', common.expectsError(sessionError)); + } client.close(); // Should be a non-op at this point }));