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

Fix: Redis race conditions with .set, .delete. and .clear when useRedisSets=true #881

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

ryanrobertsname
Copy link
Contributor

When using Redis and useRedisSets=true, it's possible to end up with a mismatch between the keys listed in the namespace set and the actual key/values in Redis.

Quick Review of Current Functionality

  • The set op takes place in two steps, the key/value is set and then the key is added to the namespace set.

  • The delete op takes place in two steps, the key/value is deleted and then the key is removed from the namespace set.

  • The clear op takes place in two steps, the keys are retrieved from the namespace set and then they are deleted (including the namespace set itself).

Problems can arise when steps become intermixed and state changes take place in between steps 1 of 2 of certain ops.

Example Problem 1 - key/value in Redis but key not listed in namespace set (actual experience)

System 1 calls set to save a value and at the same time System 2 calls clear.

Order of ops:

  1. set step 1 takes place - key/value saved
  2. clear step 1 takes place - all keys in the namespace set are retrieved (except the key from op 1 since it hasn't been added to the namespace set yet)
  3. set step 2 takes place - key is added to namespace set
  4. clear step 2 takes place - all keys retrieved from step 2 are deleted as well as the namespace set itself

The key in the namespace set gets deleted in error.

Example Problem 2 - key in namespace set, but actual key/value doesn't exist (theoretical)

  1. set step 1 takes place - key/value saved
  2. delete step 1 takes place - key/value deleted
  3. delete step 2 takes place - key deleted from namespace set
  4. set step 2 takes place - key added to namespace set

The key in the namespace set didn't get deleted, in error.

Solution

Run both set and delete in transactions.

For clear, using a transaction doesn't seem possible. As an alternative, instead of deleting the entire namespace set in step 2 we can just remove the specific keys that we retrieved from step 1. This will ensure that any new keys added after step 1 will still remain in the namespace set, but if nothing was added then the set will be empty after step 2 and automatically removed by Redis.

Please check if the PR fulfills these requirements

  • Followed the Contributing guidelines.
  • Tests for the changes have been added (for bug fixes/features) with 100% code coverage.
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? Bug fix

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 93.10% and project coverage change: -0.12% ⚠️

Comparison is base (a6bff0b) 100.00% compared to head (7237eb9) 99.88%.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #881      +/-   ##
===========================================
- Coverage   100.00%   99.88%   -0.12%     
===========================================
  Files           22       22              
  Lines         2577     2597      +20     
  Branches       339      342       +3     
===========================================
+ Hits          2577     2594      +17     
- Misses           0        3       +3     
Files Changed Coverage Δ
packages/redis/src/index.ts 98.19% <93.10%> (-1.81%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaredwray
Copy link
Owner

@ryanrobertsname Thanks so much for sending this pull request. Can you also include the additional testing to get to 100% code coverage so we can review and if all good get it merged?

@jaredwray
Copy link
Owner

@ryanrobertsname - thanks for doing this code and adding this in. I am going to merge and then add in the unit tests.

@jaredwray jaredwray merged commit cbe367c into jaredwray:main Sep 17, 2023
5 of 7 checks passed
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