From 960b9cd49882105913e2475f8b115d2960e32c82 Mon Sep 17 00:00:00 2001 From: Michael Kaufman Date: Thu, 27 Oct 2022 16:13:04 -0400 Subject: [PATCH 1/3] fix: set session as gracefully closing on goaway frame --- source/agent.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/agent.js b/source/agent.js index 922d202..1daaafa 100644 --- a/source/agent.js +++ b/source/agent.js @@ -440,6 +440,12 @@ class Agent extends EventEmitter { session.destroy(); }); + session.once('goaway', () => { + // Prevent session from being used for new requests. + // The session will eventually emit either an 'error' or 'close' event. + session[kGracefullyClosing] = true; + }); + session.once('close', () => { this._sessionCount--; From 8f8791f3b2a364994c60e5a6b538a2eb2c33ffeb Mon Sep 17 00:00:00 2001 From: Michael Kaufman Date: Fri, 28 Oct 2022 08:56:47 -0400 Subject: [PATCH 2/3] fix: session goaway handler to use gracefullyClose and updated comments --- source/agent.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/source/agent.js b/source/agent.js index 1daaafa..f8e6551 100644 --- a/source/agent.js +++ b/source/agent.js @@ -443,7 +443,20 @@ class Agent extends EventEmitter { session.once('goaway', () => { // Prevent session from being used for new requests. // The session will eventually emit either an 'error' or 'close' event. - session[kGracefullyClosing] = true; + + // See https://github.com/nodejs/node/blob/7051ba4501883955daa6bf8e442fef0c32aa5ea3/lib/internal/http2/core.js#L1166:L1175 + // Receiving a GOAWAY frame will cause the Http2Session to first emit a 'goaway' + // event notifying the user that a shutdown is in progress. If the goaway + // error code equals 0 (NGHTTP2_NO_ERROR), session.close() will be called, + // causing the Http2Session to send its own GOAWAY frame and switch itself + // into a graceful closing state. In this state, new inbound or outbound + // Http2Streams will be rejected. Existing *pending* streams (those created + // but without an assigned stream ID or handle) will be destroyed with a + // cancel error. Existing open streams will be permitted to complete on their + // own. Once all existing streams close, session.destroy() will be called + // automatically. + + gracefullyClose(session); }); session.once('close', () => { From 6ea87f9c65ef7ff8f1a94270a5ba578011a745fc Mon Sep 17 00:00:00 2001 From: Michael Kaufman <2073135+mkaufmaner@users.noreply.github.com> Date: Sun, 30 Oct 2022 10:28:08 -0400 Subject: [PATCH 3/3] chore: update comment to reference RFC --- source/agent.js | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/source/agent.js b/source/agent.js index f8e6551..1fc4fba 100644 --- a/source/agent.js +++ b/source/agent.js @@ -444,17 +444,9 @@ class Agent extends EventEmitter { // Prevent session from being used for new requests. // The session will eventually emit either an 'error' or 'close' event. - // See https://github.com/nodejs/node/blob/7051ba4501883955daa6bf8e442fef0c32aa5ea3/lib/internal/http2/core.js#L1166:L1175 - // Receiving a GOAWAY frame will cause the Http2Session to first emit a 'goaway' - // event notifying the user that a shutdown is in progress. If the goaway - // error code equals 0 (NGHTTP2_NO_ERROR), session.close() will be called, - // causing the Http2Session to send its own GOAWAY frame and switch itself - // into a graceful closing state. In this state, new inbound or outbound - // Http2Streams will be rejected. Existing *pending* streams (those created - // but without an assigned stream ID or handle) will be destroyed with a - // cancel error. Existing open streams will be permitted to complete on their - // own. Once all existing streams close, session.destroy() will be called - // automatically. + // See https://datatracker.ietf.org/doc/html/rfc7540#section-6.8 + // There is an inherent race condition between an endpoint starting new streams and the remote sending a GOAWAY frame. + // Receivers of a GOAWAY frame MUST NOT open additional streams on the connection, although a new connection can be established for new streams. gracefullyClose(session); });