Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: allow streams to complete gracefully after goaway #50202

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading