transport: Fix closing a closed channel panic in handlePing #5854
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
reviewed
Dec 9, 2022
| t.Fatalf("Failed to listen: %v", err) | ||
| } | ||
| defer lis.Close() | ||
| s := grpc.NewServer() |
There was a problem hiding this comment.
I think this needs to defer s.Stop() just in case GracefulStop() does hang (otherwise the leak detector hangs).
There was a problem hiding this comment.
Oh interesting added.
test/servertester.go
Outdated
| @@ -273,3 +273,9 @@ func (st *serverTester) writeRSTStream(streamID uint32, code http2.ErrCode) { | |||
| st.t.Fatalf("Error writing RST_STREAM: %v", err) | |||
| } | |||
| } | |||
|
|
|||
| func (st *serverTester) writeGoAwayPing(ack bool, data [8]byte) { | |||
There was a problem hiding this comment.
writePing? There's nothing about this ping that implies any relation to goaway besides the payload which is parameterized in data.
There was a problem hiding this comment.
Oh lol fair point good catch. Changed.
zasweq
changed the title
dfawley
approved these changes
Dec 13, 2022
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 #5786.
Two GoAway pings were previously closing a closed channel, this PR adds protection with respect to the drainChan closing (used as a signal to write second GoAway frame). We still to derive the underlying root cause as to why two goaway ping acks are being sent from the Swift client.
RELEASE NOTES: