From 93c4efeb25779f19af777027a3ac96df53da99f2 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 20 Oct 2023 06:28:18 -0700 Subject: [PATCH] http2: allow streams to complete gracefully after goaway A detailed analysis of the cause of this bug is in my linked comment on the corresponding issue. The primary fix is the new setImmediate call in Http2Stream#_destroy, which prevents a re-entrant call into Http2Session::SendPendingData when sending trailers after the Http2Session has been shut down, allowing the trailer data to be flushed properly before the socket is closed. As a result of this change, writes can be initiated later in the lifetime of the Http2Session. So, when a JSStreamSocket is used as the underlying socket reference for an Http2Session, it needs to be able to accept write calls after it is closed. In addition, now that outgoing data can be flushed differently after a session is closed, in two tests clients receive errors that they previously did not receive. I believe the new errors are more correct, so I changed the tests to match. Fixes: https://github.com/nodejs/node/issues/42713 Refs: https://github.com/nodejs/node/issues/42713#issuecomment-1756140062 PR-URL: https://github.com/nodejs/node/pull/50202 Reviewed-By: Matteo Collina Reviewed-By: Rafael Gonzaga --- lib/internal/http2/core.js | 7 +++++-- lib/internal/js_stream_socket.js | 5 ++++- .../test-h2-large-header-cause-client-to-hangup.js | 10 ++++++++-- .../parallel/test-http2-exceeds-server-trailer-size.js | 8 ++++++-- .../test-http2-trailers-after-session-close.js | 3 --- 5 files changed, 23 insertions(+), 10 deletions(-) rename test/{known_issues => parallel}/test-http2-trailers-after-session-close.js (89%) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 1a689859aff752..909946816c7495 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -2358,8 +2358,11 @@ class Http2Stream extends Duplex { // This notifies the session that this stream has been destroyed and // gives the session the opportunity to clean itself up. The session // will destroy if it has been closed and there are no other open or - // pending streams. - session[kMaybeDestroy](); + // pending streams. Delay with setImmediate so we don't do it on the + // nghttp2 stack. + setImmediate(() => { + session[kMaybeDestroy](); + }); callback(err); } // The Http2Stream can be destroyed if it has closed and if the readable diff --git a/lib/internal/js_stream_socket.js b/lib/internal/js_stream_socket.js index 70d6d03069f3f1..a6aee73f468b08 100644 --- a/lib/internal/js_stream_socket.js +++ b/lib/internal/js_stream_socket.js @@ -179,10 +179,13 @@ class JSStreamSocket extends Socket { // anything. doClose will call finishWrite with ECANCELED for us shortly. this[kCurrentWriteRequest] = req; // Store req, for doClose to cancel return 0; + } else if (this._handle === null) { + // If this._handle is already null, there is nothing left to do with a + // pending write request, so we discard it. + return 0; } const handle = this._handle; - assert(handle !== null); const self = this; diff --git a/test/parallel/test-h2-large-header-cause-client-to-hangup.js b/test/parallel/test-h2-large-header-cause-client-to-hangup.js index b8b08cecf6817c..eb18d9e9a20ae7 100644 --- a/test/parallel/test-h2-large-header-cause-client-to-hangup.js +++ b/test/parallel/test-h2-large-header-cause-client-to-hangup.js @@ -7,7 +7,7 @@ const http2 = require('http2'); const assert = require('assert'); const { DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE, - NGHTTP2_CANCEL, + NGHTTP2_FRAME_SIZE_ERROR, } = http2.constants; const headerSize = DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE; @@ -28,11 +28,17 @@ server.listen(0, common.mustCall(() => { function send() { const stream = clientSession.request({ ':path': '/' }); stream.on('close', common.mustCall(() => { - assert.strictEqual(stream.rstCode, NGHTTP2_CANCEL); + assert.strictEqual(stream.rstCode, NGHTTP2_FRAME_SIZE_ERROR); clearTimeout(timer); server.close(); })); + stream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR' + })); + stream.end(); } })); diff --git a/test/parallel/test-http2-exceeds-server-trailer-size.js b/test/parallel/test-http2-exceeds-server-trailer-size.js index 87c1070afbb7a4..ae2bbc1dca08bf 100644 --- a/test/parallel/test-http2-exceeds-server-trailer-size.js +++ b/test/parallel/test-http2-exceeds-server-trailer-size.js @@ -43,9 +43,13 @@ server.listen(0, () => { const clientStream = clientSession.request(); clientStream.on('close', common.mustCall()); - // These events mustn't be called once the frame size error is from the server + clientStream.on('error', common.expectsError({ + code: 'ERR_HTTP2_STREAM_ERROR', + name: 'Error', + message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR' + })); + // This event mustn't be called once the frame size error is from the server clientStream.on('frameError', common.mustNotCall()); - clientStream.on('error', common.mustNotCall()); clientStream.end(); }); diff --git a/test/known_issues/test-http2-trailers-after-session-close.js b/test/parallel/test-http2-trailers-after-session-close.js similarity index 89% rename from test/known_issues/test-http2-trailers-after-session-close.js rename to test/parallel/test-http2-trailers-after-session-close.js index 3d5be11e5b5185..f7c7387eb01380 100644 --- a/test/known_issues/test-http2-trailers-after-session-close.js +++ b/test/parallel/test-http2-trailers-after-session-close.js @@ -3,9 +3,6 @@ // Fixes: https://github.com/nodejs/node/issues/42713 const common = require('../common'); if (!common.hasCrypto) { - // Remove require('assert').fail when issue is fixed and test - // is moved out of the known_issues directory. - require('assert').fail('missing crypto'); common.skip('missing crypto'); } const assert = require('assert');