handleCall: cleanups & locking simplifications #403
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
Giving this a name makes it easier to follow what's going on here.
I don't love this, but at least it gets us to a place where the locking is reasonably simple.
lthibault
previously approved these changes
Dec 28, 2022
Per Louis's suggestion
lthibault
approved these changes
Dec 28, 2022
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.
This gets us to a place where locking in handleCall is strictly hierarchical; there is one call to syncutil.With and then later a
lock(); defer unlock().I don't love the way we're using releaseList to do the actual Recv calls, but factoring those out into one codepath turned out to be really thorny. This is at least cleaner than what we had before, and gets this into a shape where it can be easily adapted to the withLocked() design we'd discussed.
I'm also not a fan of how many codepaths have to call releaseCall, but cleaning up resource reclamation is another broader topic on my list...
This also includes a couple minor cleanups in 5617569 and 28f46de