-
Notifications
You must be signed in to change notification settings - Fork 645
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
Server might not send GOAWAY
when some streams are being processed
#2777
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pderop
changed the title
Server might not send GOAWAY when some streams are processing
Server might not send GOAWAY when some streams are being processed
Apr 20, 2023
…e all stream channels first.
In the last commit, the impact of the patch is now minimal, we only take care to sort the channelGroup in order to ensure that streams channel are handled first, before any parent server channels. |
@violetagg , can you take a look ? |
can you retake a look, I just fixed a typo, and the channel group list is not sorted anymore. |
violetagg
approved these changes
Apr 28, 2023
@violetagg , thanks. |
pderop
added a commit
that referenced
this pull request
Apr 28, 2023
violetagg
changed the title
Server might not send GOAWAY when some streams are being processed
Server might not send May 4, 2023
GOAWAY
when some streams are being processed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Sometimes, when initiating a graceful shutdown on an HTTP2 server, it may happen that some connections are closed without sending GOAWAY.
This may (not always) happen under the following conditions:
Under these conditions, the
channelGroup
that is associated to theHttpServer
contains both stream channel as well as their corresponding parent socket channels. So, when the graceful shutdown is initiated, the ServerTransport.disposeNow(Duration) method is iterating on all channels registered in the channel group.Let's say we have one http connection, and one currently active stream with id=3, so in the channelGroup, we have something like:
Now, assume that during the first iteration over the channelGroup,
disposeNow
first finds the first channel (the parent channel:NioSocketChannel with id: 0xfce41a80
), then this parent channel will be registered in thechannelsToMono
map (with an empty ArrayList).Now, for the second iteration,
disposeNow
method is now getting the second channel (the stream channel:Http2MultiplexHandler$Http2MultiplexHandlerStreamChannel with id: 0xfce41a80
. In this case the parent won't be inserted in thechannelsToMono
, because the parent has already been inserted during the first iteration.So, during the second iteration, we won't close the stream here.
But the stream is being processed and there is a channel operation which is active so the current operation will be added to the list here, and because of this the parent channel also won't be closed here, and the socket will be closed when the JVM exits without having sent a GOAWAY message.
This PR first modifies the existing
doTestGracefulShutdown
in HttpServerTests, and the method is now checking that GOAWAY are received by the client. But since map iterator ordering can vary, this test may still succeed even without any patch.Then the PR is doing a patch in the ServerTransport.disposeNow method in order to sort the channelGroup so we first handle Stream channels before handling parent ServerChannel.
Side note: when the server is configured only with H2 or with H2C, then only the stream channels (Http2MultiplexHandlerStreamChannel) are registered in the channelGroup, and the corresponding parent channels are not registered, so we don't have the problem.
However, it may be a problem to not have the parent channels registered, because in this case, if no active streams are currently being processed, then no GOAWAY message will be sent at all, which may be a problem, because if I'm correct we should always send a GOAWAY. But this is another use case, which might be addressed in another PR, if necessary.
Related to #2735