fix: Blocking commands respect canceled context #2433
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
|
@monkey92t looks reasonable WDYT? |
|
Just hit this, could this please get reviewed & merged? Looks reasonable no? |
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.
Description
This fixes an issue where blocking commands like
XReadandXReadGroupcould not be canceled using the provided context. Similarly with*PubSub.Receive.Fixes #2276
Supersedes #2432
Approach
I looked holistically at places in the code where a connection was being used without inspecting the context and applied the same basic pattern. Once a timeout is reached, the recommendation is to close the connection. Note that the change to
*PubSub.ReceiveTimeoutdoes not explicitly close the connection since that is already handled by the subsequent*PubSub.releaseConnWithLock.Note that with this change, the
ContextTimeoutEnabledclient option is redundant. Can we deprecate it?Testing
The new tests that I added all failed without this change. Note that I did not add new tests for the
processPipelineandprocessTxPipelinechanges, since pipelining blocking commands does not make sense. However, if you can think of a good test case I will be happy to add it.