grpc: ensure transports are closed when the channel enters IDLE #6620
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
Sep 12, 2023
clientconn.go
Outdated
Comment on lines
1693
to
1697
| // We have to unlock and re-lock here because GracefulClose calls | ||
| // onClose, which requires locking ac.mu. | ||
| ac.mu.Unlock() | ||
| curTr.GracefulClose() | ||
| ac.mu.Lock() |
There was a problem hiding this comment.
updateAddrs does the same kind of thing via a defer instead. Should we do that here to avoid giving up the lock and then taking it again? (Also we can remove the local variable curTr if we do this.)
Or, maybe just move this stuff to the very end of the function so we don't need to re-take the lock?
There was a problem hiding this comment.
Moving to the end was simpler and felt cleaner as well. Done.
dfawley
approved these changes
Sep 12, 2023
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.
Prior to channel idleness, sub-channels would be closed either because:
cc.ctxis canceledIn the case of the channel moving to IDLE though, we were calling
ac.tearDown, but this was not actually closing the transport because the passed in error was noterrConnDrain.With this PR, we will always close the underlying transport from
ac.tearDown, either gracefully or forcefully.RELEASE NOTES: