http2: refactor close/destroy for Http2Stream and Http2Session by jasnell · Pull Request #17406 · nodejs/node · GitHub

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

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.

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

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

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

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

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.

@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.

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

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

is this used at all?

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

can you name this function?

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?

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.

this should not be needed, why it is?

same as above :-)

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)

why this can happen? I think a comment is needed

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

can you add a comment explaining this condition?

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

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

@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…

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.

can you replace err as null?

@jasnell I don’t know if you were planning on backporting this change to v8.x, but it appears to be behind the bug fixed in #18987. I’ve added dont-land-on-v8.x for now, let me know if that’s not a good idea.

This will need to eventually get in to 8.x

…

On Feb 25, 2018 13:07, "Anna Henningsen" ***@***.***> wrote: @jasnell <https://github.com/jasnell> I don’t know if you were planning on backporting this change to v8.x, but it appears to be behind the bug fixed in #18987 <#18987>. I’ve added dont-land-on-v8.x for now, let me know if that’s not a good idea. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17406 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eUPJA56H9aqwhSkOhMgPYYAvz1M0ks5tYcuEgaJpZM4QxUEF> .

Just wondering, has there been any progress in backporting this? If not, I do see that the dont-land-on-v8.x label is still there... should it be removed so that we don't accidentally gloss over the issue?

should it be removed so that we don't accidentally gloss over the issue?

Yes definitely.

it appears to be behind the bug fixed in #18987. I’ve added dont-land-on-v8.x for now, let me know if that’s not a good idea.

@addaleax so are we okay to backport this along with #18987?

All of the http2 commits that have landed since this one are held up from being backported until this one lands so the sooner it can be done the better. :-)

Here is a list of all http tagged commits that are not on 8.x

I'm seeing if I can get a backport working for the lot

https://gist.github.com/MylesBorins/372674e5b7f5370ccf03445d819afa90

PR-URL: nodejs#17406 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

PR-URL: nodejs#17406 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> 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.

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 71d02cb Compare 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 Http2Session Dec 4, 2017

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

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

jasnell force-pushed the http2-refactor5 branch from 5ce9dad to ea67227 Compare 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 Http2Session Dec 6, 2017

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

mcollina reviewed Dec 11, 2017

View reviewed changes

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

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

mcollina reviewed Dec 12, 2017

View reviewed changes

addaleax added the dont-land-on-v8.x label Feb 25, 2018

gibfahn added lts-watch-v8.x and removed dont-land-on-v8.x labels Apr 12, 2018

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

pimterry mentioned this pull request May 11, 2026

Fix HTTP/2 RST_STREAM behaviour, add auto-drain, deprecate 'aborted', fix related compat API issues #63249

Merged

rwieruch mentioned this pull request Jul 28, 2026

connect-node: RST_STREAM(NO_ERROR) mid-response yields misleading incomplete envelope / missing status instead of a transport error connectrpc/connect-es#1751

Open

http2: refactor close/destroy for Http2Stream and Http2Session by jasnell · Pull Request #17406 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

http2: refactor close/destroy for Http2Stream and Http2Session#17406jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:http2-refactor5jasnell/node:http2-refactor5Copy head branch name to clipboard

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!

addaleax commented Feb 25, 2018

Uh oh!

jasnell commented Feb 25, 2018 via email

Uh oh!

kjin commented Apr 12, 2018

Uh oh!

gibfahn commented Apr 12, 2018

Uh oh!

jasnell commented Apr 12, 2018

Uh oh!

MylesBorins commented Apr 12, 2018

Uh oh!

Reviewers

http2: refactor close/destroy for Http2Stream and Http2Session#17406
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:http2-refactor5

jasnell commented Nov 30, 2017 •
edited
Loading

addaleax commented Dec 12, 2017 •
edited
Loading