diff --git a/doc/api/http2.md b/doc/api/http2.md index b179743d85a121..3f810168e6ff85 100644 --- a/doc/api/http2.md +++ b/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`. diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 273ddd15414b51..1c6edd65cae8f0 100644 --- a/lib/internal/http2/core.js +++ b/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) @@ -1224,6 +1228,9 @@ 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); @@ -1231,10 +1238,6 @@ function closeSession(session, code, error) { 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); @@ -1809,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](); @@ -1899,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) { diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js index ff98c23e864f74..7034f98abb6836 100644 --- a/test/parallel/test-http2-client-destroy.js +++ b/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 })); diff --git a/test/parallel/test-http2-client-session-close-before-stream-close.js b/test/parallel/test-http2-client-session-close-before-stream-close.js new file mode 100644 index 00000000000000..5ebcd8522dfb41 --- /dev/null +++ b/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(); +}));