Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URGENT Fix error in batch/transaction handling #2177

Merged
merged 8 commits into from
Jun 28, 2022
Merged

URGENT Fix error in batch/transaction handling #2177

merged 8 commits into from
Jun 28, 2022

Conversation

mgravell
Copy link
Collaborator

@mgravell mgravell commented Jun 28, 2022

A missing API override in RedisBatch/RedisTransaction can lead to some operations NOT ENTERING THE BATCH/TRANSACTION. This is very bad.

Some undefined set of operations (including at least ZRANGE) end up using what appears to be a new overload/path; this API was not overridden in the batch/transaction, with the impact that affected operations use the default behaviour and go directly to the multiplexer/connection. This means a: they are issued even if the transaction/batch is not ever executed, and b: if everything is issued, they are issued out-of-order (i.e. before the main batch/transaction)

See: #2167, #2176

@mgravell mgravell requested a review from NickCraver June 28, 2022 10:41
Previously the F+F case was getting a null default rather than the pass default value - this corrects that as well. It is not DRY across classes, but let's get this fix in ASAP then address that.
@NickCraver
Copy link
Collaborator

@mgravell looks like the default value for F+F was subtly incorrect/inconsistent as well - remedied in the NRT-friendly way (can go back and DRY up) - thoughts?

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current looks good, but I've also committed - needs cross-check eyes.

@mgravell
Copy link
Collaborator Author

mgravell commented Jun 28, 2022

Random thought; do both methods need to be virtual? Should we devirtualize the method that doesn't take the default value? One fewer thing to remember. /Cc @NickCraver

@NickCraver
Copy link
Collaborator

hmm not sure I follow (in text-only atm) - aren't we overriding it in Batch/Transaction both?

@mgravell
Copy link
Collaborator Author

hmm not sure I follow (in text-only atm) - aren't we overriding it in Batch/Transaction both?

I'm saying "what if there was only one method to override"

@NickCraver
Copy link
Collaborator

If we can make that work with NRTs all for it - but I don't think you can with the semantics of Task<T> vs Task<T?> unless you basically throw away most of the checks they enforce today. I tried for a while, but couldn't simplify further (more than happy to be proven wrong here). I do think we can DRY them up a bit though - was going to go for that in another PR to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants