Repository navigation
[v0.20.x-branch] Backport #11329: peer: retire cooperative closers without stranding callers - #11348
Draft
github-actions[bot] wants to merge 1 commit into
Draft
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Author
|
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-11329-to-v0.20.x-branch
git worktree add --checkout .worktree/backport-11329-to-v0.20.x-branch backport-11329-to-v0.20.x-branch
cd .worktree/backport-11329-to-v0.20.x-branch
git reset --hard HEAD^
git cherry-pick -x 9b7a98284b2a7916b2cfdb43c2aac198fc5d2f38 5d6a97d5ac056a81d5ad689aab98bbe50b00ebc9 66ffdd053696ddb841cf1a6ac2fbab94d327ee37 804a2394285569a7bff31c79d720eaf1e853cb17 5e23f9ab357ec573f2ef0e990d7d7a59f3beaad4 79497bb567b2fde3eb4db318f6e51ff4f74a7f8c
git push --force-with-lease |
5 of 6 tasks
This file contains hidden or 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
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.
Backport of #11329
Change Description
In this PR, we retire any active cooperative-close state whenever
WipeChannelremoves a channel from the peer and switch, and we make retiring a closer tell the caller who was waiting on it on every path that retires one.A locally initiated close whose closer was retired left its caller waiting forever.
WipeChanneldeletes the closer, and the closer is the only writer of a close request's error, so a close whose channel hit a non-disconnecting link failure leftCloseChannelwaiting on a request nobody was left to answer, parking until the daemon shut down. A peer disconnect did the same, for both closer variants. Retiring a closer now answers the caller, and the notification is parameterized by the reason, since what ends a close is not always a link failure.A close request's error was a one-slot channel with several writers. Two of those writes selected on nothing else, so once the caller had stopped reading they blocked where they stood, and one of them runs on the channel manager's goroutine, where a block stops every operation for that peer. Adding the disconnect notification introduced a second writer, so the full-buffer case can now occur.
The request now carries a promise rather than a channel. Resolving it is idempotent and reports whether this call was the one that resolved it, so a path arriving second is refused instead of overwriting the outcome the caller already has, and it cannot block, so a path resolving a close is never held up by a caller that has gone away. Callers that wait on channels see no change: the promise is awaited on their behalf and only a failure is forwarded, since a close that succeeds reports through its updates.
One design point worth stating, since it is what makes the multi-caller case correct rather than lucky: a channel can have more than one caller while an earlier one is still live, because a fee bump registers a second. It is the registry's mutex-protected removal, not the promise's idempotence, that partitions ownership of a caller's updates — the promise guards the error, and the exclusive take is what stops two paths from delivering to one caller.
We also require
IsChannelCleanbefore the legacy closer treats a missing link as evidence that the channel has flushed, since link removal also happens on paths that do not retire the closer.Steps to Test
Pull Request Checklist
Testing
Code Style and Documentation