Skip to content

Commit

Permalink
http2: allow streams to complete gracefully after goaway
Browse files Browse the repository at this point in the history
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: #42713
Refs: #42713 (comment)
PR-URL: #50202
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
  • Loading branch information
murgatroid99 authored and targos committed Oct 23, 2023
1 parent c55f8f3 commit 3f8cbb1
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 10 deletions.
7 changes: 5 additions & 2 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/js_stream_socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
10 changes: 8 additions & 2 deletions test/parallel/test-h2-large-header-cause-client-to-hangup.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}
}));
8 changes: 6 additions & 2 deletions test/parallel/test-http2-exceeds-server-trailer-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down

0 comments on commit 3f8cbb1

Please sign in to comment.