http2: refactor close/destroy for Http2Stream and Http2Session#17406

jasnell · 2017-11-30T22:40:53Z

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:

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)
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.
Http2Stream.prototype.rstStream() has been renamed to Http2Stream.prototype.close() and the various rstWith.... aliases have been removed.
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

jasnell · 2017-12-04T23:58:05Z

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 · 2017-12-05T17:55:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11894/

jasnell · 2017-12-05T18:14:26Z

Trying again: https://ci.nodejs.org/job/node-test-pull-request/11895/

jasnell · 2017-12-05T20:08:18Z

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

jasnell · 2017-12-06T02:54:48Z

Good CI: https://ci.nodejs.org/job/node-test-commit/14590/

apapirovski · 2017-12-06T15:46:02Z

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 · 2017-12-06T23:09:04Z

@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 · 2017-12-06T23:10:04Z

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

mcollina

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

mcollina · 2017-12-11T07:51:44Z

lib/internal/http2/core.js

is this used at all?

mcollina · 2017-12-11T07:52:56Z

lib/internal/http2/core.js

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

mcollina · 2017-12-11T07:54:23Z

lib/internal/http2/core.js

can you name this function?

mcollina · 2017-12-11T07:55:46Z

lib/internal/http2/core.js

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?

mcollina · 2017-12-11T07:57:00Z

lib/internal/http2/core.js

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.

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.

mcollina · 2017-12-11T08:01:43Z

lib/internal/http2/core.js

this should not be needed, why it is?

same as above :-)

mcollina · 2017-12-11T08:03:26Z

lib/internal/http2/core.js

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

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)

mcollina · 2017-12-11T08:04:25Z

lib/internal/http2/core.js

why this can happen? I think a comment is needed

mcollina · 2017-12-11T08:04:46Z

lib/internal/http2/core.js

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

mcollina · 2017-12-11T08:05:11Z

lib/internal/http2/core.js

can you add a comment explaining this condition?

jasnell · 2017-12-11T21:00:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12043/

jasnell · 2017-12-12T03:44:06Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/12053/

addaleax · 2017-12-12T11:51:21Z

@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 · 2017-12-12T13:27:11Z

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.

mcollina · 2017-12-12T15:08:28Z

lib/internal/http2/compat.js

can you replace err as null?

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

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.

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]>

Improve documentation of callback signature of http2Stream.pushStream() function to align with the changes made in nodejs#17406. PR-URL: nodejs#18258Fixes: nodejs#18198 Refs: nodejs#17406 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

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

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.

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]>

Improve documentation of callback signature of http2Stream.pushStream() function to align with the changes made in #17406. Backport-PR-URL: #20456 PR-URL: #18258Fixes: #18198 Refs: #17406 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Improve documentation of callback signature of http2Stream.pushStream() function to align with the changes made in nodejs#17406. PR-URL: nodejs#18258Fixes: nodejs#18198 Refs: nodejs#17406 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

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]>

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]>

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]>

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]>

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 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 force-pushed the http2-refactor5 branch 4 times, most recently from b531e54 to 71d02cbCompare December 4, 2017 23:56

jasnell requested a review from mcollina December 4, 2017 23:56

jasnell changed the title ~~[WIP] http2: refactor close/destroy for Http2Stream and Http2Session~~http2: refactor close/destroy for Http2Stream and Http2SessionDec 4, 2017

jasnell force-pushed the http2-refactor5 branch from 71d02cb to 4886232Compare December 5, 2017 17:54

jasnell force-pushed the http2-refactor5 branch from 1912f41 to dc812e1Compare December 6, 2017 02:52

jasnell force-pushed the http2-refactor5 branch from 5ce9dad to ea67227Compare December 6, 2017 17:40

jasnell changed the title ~~http2: refactor close/destroy for Http2Stream and Http2Session~~[WIP] http2: refactor close/destroy for Http2Stream and Http2SessionDec 6, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086ddCompare December 8, 2017 16:37

mcollina reviewed Dec 11, 2017
View reviewed changes

jasnell force-pushed the http2-refactor5 branch from 44548d5 to c288deeCompare December 11, 2017 17:54

jasnell force-pushed the http2-refactor5 branch from 9bbf47e to a2f5486Compare December 12, 2017 03:40

mcollina reviewed Dec 12, 2017
View reviewed changes

lib/internal/http2/compat.js Outdated
Copy link
Member
mcollinaDec 12, 2017
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?

MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged

Flarna mentioned this pull request May 16, 2018
http2: stream.pushStream callback signature changed in v8.11.2 #20773
Closed

ChALkeR mentioned this pull request Jun 23, 2018
doc: fix http2stream.pushStream error doc #21487
Merged
4 tasks

Flarna mentioned this pull request Dec 2, 2018
http2: [8.x] server stream emits error after close #24559
Closed

Addono mentioned this pull request Feb 3, 2019
Node.js changed HTTP2 API RisingStack/http2-push-example#6
Open

mildsunrise mentioned this pull request May 26, 2019
http2: close not emitted when receiving RST_STREAM after DATA #27863
Closed

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Uh oh!

http2: refactor close/destroy for Http2Stream and Http2Session#17406

http2: refactor close/destroy for Http2Stream and Http2Session #17406

Uh oh!

Conversation

jasnell commented Nov 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Dec 4, 2017

Uh oh!

jasnell commented Dec 5, 2017

Uh oh!

jasnell commented Dec 5, 2017

Uh oh!

jasnell commented Dec 5, 2017

Uh oh!

jasnell commented Dec 6, 2017

Uh oh!

apapirovski commented Dec 6, 2017

Uh oh!

jasnell commented Dec 6, 2017

Uh oh!

apapirovski commented Dec 6, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 11, 2017

Uh oh!

jasnell commented Dec 12, 2017

Uh oh!

addaleax commented Dec 12, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jasnell commented Nov 30, 2017•
edited
Loading

addaleax commented Dec 12, 2017•
edited
Loading