🧵 Refactor thread synchronization for sending commands#692
Merged
Conversation
Sending a command must be synchronized: * so multiple threads aren't sending simultaneously * so sending the command is correctly interleaved with response handlers But the command's arguments are presumed to be non-shared (or thread-safe). And this validation does not reference any connection state. So the command arguments can be validated _before_ locking.
Now that the deadlock condition in `#disconnect` has been fixed, it's better to handle the invalid response error before releasing the lock.
nevans
changed the title
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
1 participant
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.
Related to #693, but that handles code that executes in the receiver thread and this handles code that executes in client threads. (Also, this PR is significantly simpler than that one.)
Sending a command must be synchronized:
But the command's arguments are presumed to be non-shared (or thread-safe), and this validation does not reference any connection state. Also, now that the deadlock condition in
#disconnecthas been fixed (#686), it's better to handle the invalid response error before releasing the lock.So, this updates argument validation to run prior to locking and invalid response error handling to run inside the lock.