http2: fix graceful session close by lundibundi · Pull Request #30854 · nodejs/node · GitHub

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

This is tied to
started with #20630, then #19852 and #20772.

The description is in the commits. This should fix our ECONNRESET during graceful close bugs in http2 tests.

Basically now we use socket.end() upon graceful close.

Also, I understand that now there is no way to actually destroy the socket if needed. If the second commit is accepted I planned to 'overload' session.destroy() to actually destroy the socket if we already initiated graceful close but want to destroy the session without waiting for FIN.

/cc @addaleax @jasnell @apapirovski @nodejs/http2

LGTM with green CI

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

Also, this doesn't unset SESSION_STATE_READING_STOPPED from flags_ though I'm not sure if we should do it as we are basically done with the session. Perhaps we should at least for correctness?

Hm, why do we start reading again here? But yes, I think this should unset that flag as we do use that to keep track of whether we’re reading or not

That is in order to actually receive the FIN packet from the other party after .end().

Now after the session is closed we call ReadStart() on the underlying
stream to allow socket to receive the remaining data and FIN packet.
Previously only ReadStop() was used therefore blocking the receival of
FIN by the socket and 'end' event after .end() call.

The handle will be stopped by the .destroy() in the net's _final after we receive the FIN packet.

I don't think the destroyed check is necessary here

Also, the error would be potentially swallowed here

NOTE, socket.destroy could schedule an error after the emitClose below. I'm not exactly sure what happens on socket error here. Note sure if relevant.

This part stayed the same as before. I think an error will be caught by socketOnError and ignored because socket[kSession] is now undefined.

Ah, I see. Next question though. Should that error be ignored? There is no point in passing error to destroy(error) if it's going to be ignored anyway.

I guess it's for 'correctness' reasons only, like 'we destroy the socket because of this error'.
As for the consequent errors, I think it's fine to ignore, we have already closed the session (and possibly even destroyed) so we don't care about the socket anymore as long as it closes.

NOTE, if this is going to be backported to v12, end(cb) doesn't always invoke the callback. Note sure if relevant.

Also, the callback may or may not (depending on Node version) be invoked with an error. Might cause emitClose to be invoked twice? Once through here and once through error handler?

This may be an issue. I think we can manually do once('finish'), once('error') for v12, would that be okay?

Once through here and once through error handler?

Could you clarify what error handler? If you refer to socketOnError then that is basically noop after cleanupSession() as we remove socket[kSession].

Also, this raises another question, previously any error from socket would be ignored in here, should we perhaps forward it to emitClose now?

I think we can manually do once('finish'), once('error') for v12, would that be okay?

Yes, I think so.

Also, this raises another question, previously any error from socket would be ignored in here, should we perhaps forward it to emitClose now?

Yes, I think so since.

oop after cleanupSession() as we remove socket[kSession]

Actually, this might be a problem for v13 as well, not sure where/when the end(cb) fix was merged.

#29747

Not familiar enough with http2/core but left some minor comments that might or might not be of relevance.

Hm, why do we start reading again here? But yes, I think this should unset that flag as we do use that to keep track of whether we’re reading or not

Windows CI is showing a lot of timeouts on http2 tests. Is this something you can address, @lundibundi?

@Trott yeah, that is connected to different timings of windows which leads to TCP handle just hanging there even after shutdown (and session GC) or destoy happening too soon so that the socket doesn't get the chance to write even the error code we provided. I was able to partially solve that by moving finishDestroy back to setImmediate.
Also, there is some issue on windows where if I use ReadStart it just indefinitely waits if the other party just .destroy the socket (even though we receive the ECONNRESET). I'm not sure but I assume that is as expected or perhaps I'm doing something wrong? I'm not that familiar with libuv but a quick glance showed some sort of 'active handle counting' on windows upon uv_read_start which may be why I'm getting a dangling socket IIUC.
Anyway will hopefully get time to investigate this further in a few days. Will appreciate any help regarding these issues.

@lundibundi I can spend a little bit of time digging into this over the next week. I'll need to dig up my notes but I went in this direction at one point and abandoned it because of the same Windows problems. 😞

The little I do recall, there's not the same contract on Windows where you can be certain that your writes have flushed to the socket even after you're told by the OS (well, libuv) that the write has flushed.

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks.

Well, errors on Windows are somewhat fixable but it results in a lot of flags_ & SESSION_STATE_CLOSED checks where we use ReadStart\ReadStop and also relies on the timing of the calls (i.e. only make ondone callback from Http2Session::Close in the setImmediate from C++ to only then use ReadStart() which both doesn't guarantee write finish and may lead to segfaults as we have to capture the session which may be destroyed on the next loop). Though it makes other issues pop up - event loop doesn't stop if the other party really destroyed the socket and didn't send anything which we obviously do in some tests.
Therefore I decided to drop that juggling for now and stored that in a separate branch http2-graceful-session-close-proper, I may push my for-windows changes later if need.

The second commit now changes the close() and destroy() to both wait for Writable side of the socket to finish and only then destroy() the socket so that we won't be stuck and always get 'close' on socket. I think this is okay for both close and destroy as after we have closed the session nghttp shouldn't allow sending any more data IIUC and we will just write pending data and finish. This will make close more robust but more importantly, we will now always have our goaway frame upon destroy sent to the other party and not just ECONNRESET if the timings are unlucky. This is somewhat contradictory to the destroy idea so if this is not acceptable then I'll separate the code path of close and destroy to only have this behavior for graceful close and destroy will go back to setImmediate because without it on Windows ECONNRESET happens like 90% of the time. Though then, I think we should at least allow users to reliably send the specific goaway code/error in the close call (I think it is possible right now if we do session.goaway(...) + session.close() though not as intuitive).

ping @mcollina @addaleax as this has your approvals, the implementation changed slightly to avoid issues with Windows for now. The proper explanation is in the comment above. (forgot to ping there)

still LGTM.

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

Landed in a8c2c66, be3c7ac 🎉

PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This PR makes test/parallel/test-http2-reset-flood.js very flaky on v12.x-staging. I'm marking it backport-requested because some investigation is required.

This PR makes test/parallel/test-http2-reset-flood.js very flaky on v12.x-staging. I'm marking it backport-requested because some investigation is required.

I wonder if it's the same V8 issue as #31107.

PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. PR-URL: nodejs#30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This slightly alters the behaviour of session close by first using .end() on a session socket to finish writing the data and only then calls .destroy() to make sure the Readable side is closed. This allows the socket to finish transmitting data, receive proper FIN packet and avoid ECONNRESET errors upon graceful close. onStreamClose now directly calls stream.destroy() instead of kMaybeDestroy because the latter will first check that the stream has writableFinished set. And that may not be true as we have just (synchronously) called .end() on the stream if it was not closed and that doesn't give it enough time to finish. Furthermore there is no point in waiting for 'finish' as the other party have already closed the stream and we won't be able to write anyway. This also changes a few tests to correctly handle graceful session close. This includes: * not reading request data (on client side) * not reading push stream data (on client side) * relying on socket.destroy() (on client) to finish server session due to the destroy of the socket without closing the server session. As the goaway itself is *not* a session close. Added few 'close' event mustCall checks. Backport-PR-URL: #34845 PR-URL: #30854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

lundibundi requested review from addaleax, apapirovski and jasnell December 8, 2019 14:40

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 8, 2019

lundibundi added the http2 Issues or PRs related to the http2 subsystem. label Dec 8, 2019

This comment has been minimized.

Sign in to view

mcollina approved these changes Dec 8, 2019

View reviewed changes

lundibundi commented Dec 8, 2019

View reviewed changes

ronag reviewed Dec 8, 2019

View reviewed changes

addaleax approved these changes Dec 9, 2019

View reviewed changes

Flarna mentioned this pull request Dec 10, 2019

test: fix flaky test-http2-client-upload #29889

Closed

3 tasks

lundibundi added 2 commits December 16, 2019 19:31

http2: wait for session to finish writing before destroy

0995c29

lundibundi force-pushed the http2-graceful-session-close branch from ac665ae to 082c0cb Compare December 16, 2019 17:44

ronag reviewed Dec 16, 2019

View reviewed changes

Comment thread lib/internal/http2/core.js

jasnell approved these changes Dec 18, 2019

View reviewed changes

mcollina approved these changes Dec 21, 2019

View reviewed changes

lundibundi added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 21, 2019

Trott approved these changes Dec 22, 2019

View reviewed changes

BridgeAR closed this Dec 25, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos added the backport-requested-v12.x label Jan 14, 2020

targos added backport-open-v12.x and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-requested-v12.x labels Apr 25, 2020

targos mentioned this pull request Apr 25, 2020

[v12.x] Backport http2: fix graceful session close #33059

Closed

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

codebytere mentioned this pull request May 18, 2021

[Bug]: http2 can cause SIGPIPE on client close electron/electron#29206

Closed

3 tasks

+                  Debug(this, "make done session callback");
+                  HandleScope scope(env()->isolate());
+                  MakeCallback(env()->ondone_string(), 0, nullptr);
+                  if (stream_ != nullptr) stream_->ReadStart();

+                  Debug(this, "make done session callback");
+                  HandleScope scope(env()->isolate());
+                  MakeCallback(env()->ondone_string(), 0, nullptr);
+                  if (stream_ != nullptr) stream_->ReadStart();

+                if (handle) handle.ondone = null;
+                cleanupSession(session);
+                if (socket && !socket.destroyed)

+                cleanupSession(session);
+                if (!socket.destroyed) {
+                  socket.end(() => emitClose(session));

http2: fix graceful session close by lundibundi · Pull Request #30854 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: fix graceful session close - #30854

http2: fix graceful session close#30854lundibundi wants to merge 2 commits intonodejs:masternodejs/node:masterfrom lundibundi:http2-graceful-session-closelundibundi/node:http2-graceful-session-closeCopy head branch name to clipboard

Conversation

lundibundi commented Dec 8, 2019

Checklist

Uh oh!

This comment has been minimized.

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 8, 2019

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!

ronag Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ronag Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ronag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 11, 2019

Uh oh!

lundibundi commented Dec 11, 2019

Uh oh!

apapirovski commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lundibundi commented Dec 16, 2019

Uh oh!

Uh oh!

lundibundi commented Dec 20, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 21, 2019

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

targos commented Jan 14, 2020

Uh oh!

Trott commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

http2: fix graceful session close#30854
lundibundi wants to merge 2 commits into
nodejs:masterfrom
lundibundi:http2-graceful-session-close

ronag Dec 8, 2019 •
edited
Loading

ronag Dec 8, 2019 •
edited
Loading

ronag left a comment •
edited
Loading

apapirovski commented Dec 14, 2019 •
edited
Loading