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: fix graceful session close #30854

Closed

Conversation

lundibundi
Copy link
Member

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

@nodejs-github-bot 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 lundibundi added the http2 Issues or PRs related to the http2 subsystem. label Dec 8, 2019
@nodejs-github-bot

This comment has been minimized.

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.

LGTM with green CI

@nodejs-github-bot
Copy link
Collaborator

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

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (handle) handle.ondone = null;
cleanupSession(session);

if (socket && !socket.destroyed)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the destroyed check is necessary here

Copy link
Member

Choose a reason for hiding this comment

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

Also, the error would be potentially swallowed here

cleanupSession(session);

if (socket && !socket.destroyed)
socket.destroy(error);
Copy link
Member

@ronag ronag Dec 8, 2019

Choose a reason for hiding this comment

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

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.

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 part stayed the same as before. I think an error will be caught by socketOnError and ignored because socket[kSession] is now undefined.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

cleanupSession(session);

if (!socket.destroyed) {
socket.end(() => emitClose(session));
Copy link
Member

@ronag ronag Dec 8, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Copy link
Member

Choose a reason for hiding this comment

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

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]

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@Trott
Copy link
Member

Trott commented Dec 11, 2019

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

@lundibundi
Copy link
Member Author

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

@apapirovski
Copy link
Member

apapirovski commented Dec 14, 2019

@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.
@lundibundi
Copy link
Member Author

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

@lundibundi
Copy link
Member Author

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)

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.

still LGTM.

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2019
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

Landed in a8c2c66, be3c7ac 🎉

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos
Copy link
Member

targos commented Jan 14, 2020

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.

@Trott
Copy link
Member

Trott commented Jan 15, 2020

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.

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added backport-open-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request Sep 22, 2020
PR-URL: nodejs#30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request Sep 22, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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