Added a GOAWAY frame send on client transport shutdown #4248
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
dfawley
requested changes
Mar 9, 2021
| // The HTTP/2 spec mentions that a GOAWAY frame should be sent before a connection close. If this close() function | ||
| // ever starts to take in an HTTP/2 error code the peer will be able to get more information about the reason | ||
| // behind the connection close. | ||
| if err := t.framer.fr.WriteGoAway(math.MaxUint32, http2.ErrCodeNo, []byte{}); err != nil { |
There was a problem hiding this comment.
If writing the GOAWAY fails, we still need to close the conn and do the other cleanup below, like adding an error to any streams that are still open.
| // The HTTP/2 spec mentions that a GOAWAY frame should be sent before a connection close. If this close() function | ||
| // ever starts to take in an HTTP/2 error code the peer will be able to get more information about the reason | ||
| // behind the connection close. | ||
| if err := t.framer.fr.WriteGoAway(math.MaxUint32, http2.ErrCodeNo, []byte{}); err != nil { |
There was a problem hiding this comment.
It's possible we want to do some refactoring here. I think I was expecting we'd pass an error to http2Client.Close, and that would dictate the code used here -- OR, pass the code in directly. Anywhere we already send a GOAWAY, we could delete that and rely on this to do it instead. Or we need a way to call Close to indicate a GOAWAY was already sent, and so Close should not send another one.
|
Closing this PR now. Let's get back to this when it's ready. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes issue #460. The data of the GOAWAY frame is that of a peer attempting a graceful shutdown according to the HTTP/2 spec (ErrCodeNo, Stream ID of 2 ^ 32 - 1). Eventually, seen by Doug's comment in #4190, it is desirable to pass an error into http2_client.close(), which will end up in the GOAWAY frame, but for now any GOAWAY frame sent is better than none.