Tweak locking around Conn.sender #401
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
We now expect the caller to be holding the lock. This makes more sense since the other place we use c.sender is also holding the lock. This lets us get rid of another Unlock/Lock pair, which was unnecessary. Future work includes moving c.sender inside c.lk, since we never actually send when not holding the lock anymore.
Specifically: - Split it into separate fields for Rx and Tx - Move the send end inside Conn.lk - Use spsc instead of mpsc, now that it's protected by Conn.lk
- Removed an unlock/lock pair, which was now incorrect since we changed the contract on answer.sendMsg to require holding the lock. This also removes a deadlock in the event that the finish had already been received, since we were re-locking twice; both calls to Lock() have been removed. - Removed a duplicate call to ans.destory; in the case where the finish had already been received we would have (but for the deadlock) called this twice, once in the select and once at the end of the function. I suspect there was supposed to be a return after the first call, but it's cleaner to just do it in one place anyway.
|
Tacked on one more fix; see the commit message on the last commit. |
lthibault
approved these changes
Dec 27, 2022
There was a problem hiding this comment.
Looks good. I really like the way this lock-reduction effort is panning out overall 👍
Quick note: I'm going to let you hit the merge button until we get around to fixing CI. I know most of these failures are known/probably-harmless issues, but I'd rather leave the final decision to merge to you.
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.
Contrary to the comment, there was no actual need to not be holding the lock when calling answer.sendMsg, and flipping the contract allows us to remove a superfluous unlock/lock pair.
This also changes the send queue back to using
spscand protects it with Conn.lk, since we no longer add to the send queue when we're not holding the connection lock anyway. We'd tried this in the past, but backed it out due to the now-removed unlock/lock pair.Doing this required splitting it into two separate fields, so the receive end could still be outside Conn.lk.