Fix moar deadlocks#243
Merged
Merged
Conversation
...just for the remainder of handleReturn off into its own goroutine. This could use some cleanup -- it's a bit weird that we're holding the lock and then unlocking it in the goroutine, though I've stared at it and it's correct. But it should be made clearer.
...by introducing a far less serious bug. See the comments. This also fixes *another* deadlock, which is far simpler and dumber, and was in hindsight probably what we were actually seeing when debugging this: I noticed while making this change we were treating the returned ReleaseFunc as an *error*, and checking that it was nil before rigging up the gotResponse() call -- so it would just deterministically not ever clear used quota. *Facepalm* But hey, I found a sublter, more insidious bug while hunting down a stupid, deterministic one. That's always nice.
lthibault
approved these changes
May 15, 2022
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
2 participants
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.
The results of the debugging session @lthibault and I had the other day.
There are actually three separate deadlock bugs, only two of which we noticed the other day, the third of which we didn't notice but was probably what we were actually observing the symptoms of, and which was way way dumber than the other two and we fixed without even noticing -- see the commit messages.
Note that the diff for rpc.go is an abomination; all it's doing is wrapping a huge block of code in
go func() { ... } ()and adding a comment, but this simple change is over git's head apparently.