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: refactor close/destroy for Http2Stream and Http2Session #17406

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 30, 2017

This is a significant refactoring of the close/destroy flow and API for Http2Stream and Http2Session. There is quite a bit more to do in here, it's a bit slow going because the flow is rather complex and I'm trying not to break too much as I go.

There are several important bits:

  1. Previously, the destroy operations for both Http2Session and Http2Stream were executed over multiple nextTick and setImmediate hops. Now the objects are unusable immediately when calling destroy() and we're using env->SetImmediate() to handle the need to defer final cleanup (cc @addaleax)

  2. This reworks the destroy and error handling flow between the Http2Session and socket and eliminates the 'socketError' event. Errors occurring on the socket are forwarded to the error event on the associated Http2Session. On the server, those are forwarded to the 'sessionError' event.

  3. Http2Stream.prototype.rstStream() has been renamed to Http2Stream.prototype.close() and the various rstWith.... aliases have been removed.

  4. An improved Http2Session.prototype.close() has been implemented, allowing better API symmetry.

There are still a few more todo's that need to be handled here. Specifically, proper handling of last stream ID on a goaway frame, but this is already a sizable chunk of work. Let's get this reviewed and landed and I'll keep pushing forward.

/cc @nodejs/http2

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 30, 2017
@jasnell jasnell added http2 Issues or PRs related to the http2 subsystem. wip Issues and PRs that are still a work in progress. labels Nov 30, 2017
@jasnell jasnell force-pushed the http2-refactor5 branch 4 times, most recently from b531e54 to 71d02cb Compare December 4, 2017 23:56
@jasnell jasnell requested a review from mcollina December 4, 2017 23:56
@jasnell jasnell changed the title [WIP] http2: refactor close/destroy for Http2Stream and Http2Session http2: refactor close/destroy for Http2Stream and Http2Session Dec 4, 2017
@jasnell
Copy link
Member Author

jasnell commented Dec 4, 2017

Ping @nodejs/http2 ... this should be ready for review. It's a rather large and complicated set of changes that refactors the shutdown, destroy and cleanup logic in Http2Stream and Http2Session. It's not going to be trivial to review.

@mcollina ... Please take a good careful look at this.

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2017

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2017

@jasnell
Copy link
Member Author

jasnell commented Dec 5, 2017

Ok, there's definitely still quite a bit of wonkiness in here that I'll have to chase down.

@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2017

@apapirovski
Copy link
Member

I'll start reviewing this over the next few days. Might take a little while to get through. I would also like to run this against the test suite I have for express & related middleware.

@jasnell jasnell changed the title http2: refactor close/destroy for Http2Stream and Http2Session [WIP] http2: refactor close/destroy for Http2Stream and Http2Session Dec 6, 2017
@jasnell
Copy link
Member Author

jasnell commented Dec 6, 2017

@apapirovski ... @mcollina and I discussed this a bit today and worked through a few more changes that need to be made (which will actually undo some of the changes made in this PR so far...). I'm working on those now and should hopefully have it complete by end of the day tomorrow at the latest.

@apapirovski
Copy link
Member

@jasnell No worries. Keep me posted, happy to start reviewing whenever it's ready.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good progress, I hope I reviewed all of it. Let me know if I missed some critical changes.

@@ -66,10 +66,11 @@ const TLSServer = tls.Server;
const kInspect = require('internal/util').customInspectSymbol;

const kAuthority = Symbol('authority');
const kDestroySocket = Symbol('destroy-socket');
const kErrorEmitted = Symbol('error-emitted');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used at all?

owner[kUpdateTimer]();
process.nextTick(emit, owner, 'error', error);
if (state.fd !== undefined)
fs.close(state.fd, () => {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we swallowing an error here? Can we destroy those in parallel safely?

// indicate if the ping was successful or not. The duration indicates the
// number of milliseconds elapsed since the ping was sent and the ack
// received. The payload is a Buffer containing the 8 bytes of payload
// data received on the PING acknowlegement.
function pingCallback(cb) {
return function(ack, duration, payload) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you name this function?


session.settings(settings);
process.nextTick(emit, session, 'connect', session, socket);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of code that is kind of hot. This should live in a top level function somewhere. Can we remove the need to wrap it in a closure?

if (state.shutdown || state.shuttingDown)
// Destroy the Http2Session, making it no longer usable and cancelling
// any pending activity.
destroy(error = NGHTTP2_NO_ERROR, code, lastStreamID, opaqueData) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need all these arguments? I would really prefer we did a .destroy(err, callbak), where err  is an instance of Error. It seems this signature is one for a more internal function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the lastStreamIDand opaqueData here.. I've removed those. The code is required because we have to know which error code to send in the final GOAWAY frame and that cannot always be determined automatically.

// case, drop the data on the floor. An error should have already been
// emitted.
if (this.destroyed)
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be needed, why it is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above :-)

// the Http2Stream to be closed and ultimately destroyed. After calling
// close, it is still possible to queue up PRIORITY and RST_STREAM frames,
// but no DATA and HEADERS frames may be sent.
close(code = NGHTTP2_NO_ERROR, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be close(callback)? I would prefer to not expose NGHTTP2 error codes. For errors, we have destroy().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an NGHTTP2 error code, it's an HTTP2 RST_STREAM error code. An RST_STREAM is used to close the stream here so we need to know what code to use. There are legitimate cases where the user might specify something other than NO_ERROR without using destroy(err)

return;
}
if (err) {
process.nextTick(emit, this, 'error', err);
if (this.destroyed || this.closed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this can happen? I think a comment is needed

process.nextTick(emit, this, 'error', err);
if (this.destroyed || this.closed) {
// TODO(jasnell): emit error here instead?
abort(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silently ignoring things is not nice, I'd do the error here.

if (!Array.isArray(headersList)) {
process.nextTick(emit, this, 'error', headersList);
if ((typeof options.statCheck === 'function' &&
options.statCheck.call(this, stat, headers, statOptions) === false) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment explaining this condition?

@jasnell
Copy link
Member Author

jasnell commented Dec 11, 2017

@jasnell
Copy link
Member Author

jasnell commented Dec 12, 2017

@addaleax
Copy link
Member

addaleax commented Dec 12, 2017

@jasnell Can you give an overview over what’s going to happen with this PR? Is it (nearly) ready? :)

And maybe, are the C++ bits independent enough from the rest that they could be split off into their own PR? I’ll continue working on StreamBase after #17564 and I think that might be nice to have this landed before that…

@jasnell
Copy link
Member Author

jasnell commented Dec 12, 2017

This pr is nearly there. Working on getting one final test working then will be squashing and reorganizing the commits down. Unfortunately, no, the c++ bits cannot be spun off into their own pr. The goal is to get this finished up today.

callback(err);
return;
}
callback(err, new Http2ServerResponse(stream));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you replace err as null?

kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
PR-URL: nodejs#17406
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

Backport-PR-URL: nodejs#18050
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Backport-PR-URL: nodejs#18050
PR-URL: nodejs#17406
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

This is a significant cleanup and refactoring of the
cleanup/close/destroy logic for Http2Stream and Http2Session.
There are significant changes here in the timing and ordering
of cleanup logic, JS apis. and various related necessary edits.
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Refs: nodejs#17406 (comment)

PR-URL: nodejs#18088
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in nodejs#17406.

PR-URL: nodejs#18258
Fixes: nodejs#18198
Refs: nodejs#17406
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #20456
PR-URL: #17406
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

Backport-PR-URL: #18050
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17406
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>

This is a significant cleanup and refactoring of the
cleanup/close/destroy logic for Http2Stream and Http2Session.
There are significant changes here in the timing and ordering
of cleanup logic, JS apis. and various related necessary edits.
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Refs: #17406 (comment)

Backport-PR-URL: #20456
PR-URL: #18088
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in #17406.

Backport-PR-URL: #20456
PR-URL: #18258
Fixes: #18198
Refs: #17406
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in nodejs#17406.

PR-URL: nodejs#18258
Fixes: nodejs#18198
Refs: nodejs#17406
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Jul 10, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request #17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by #21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: #21470
Refs: #17406
PR-URL: #21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request #17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by #21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: #21470
Refs: #17406
Backport-PR-URL: #22850
PR-URL: #21487
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants