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

MSETNX must not set keys if any of them exists #980

Closed
prvyk opened this issue Jan 30, 2025 · 3 comments · Fixed by #996
Closed

MSETNX must not set keys if any of them exists #980

prvyk opened this issue Jan 30, 2025 · 3 comments · Fixed by #996

Comments

@prvyk
Copy link
Contributor

prvyk commented Jan 30, 2025

Describe the bug

When msetnx gets several keys and one of them already exists, msetnx still sets the nonexisting keys. The redis page is very clear:

"MSETNX will not perform any operation at all even if just a single key already exists."

https://redis.io/docs/latest/commands/msetnx/

Steps to reproduce the bug

msetnx a 0 b 1
msetnx c 2 a 3

get a => 0
get c => 2

Expected behavior

No response

Screenshots

No response

Release version

No response

IDE

No response

OS version

No response

Additional context

No response

@badrishc
Copy link
Contributor

The Mxxx commands in Garnet are based on non-transactional semantics, i.e. they are treated as a sequence of individual commands. If users need transactional semantics for the entire batch, they can write this as a custom stored procedure. This is documented here (it says MSET, that should be edited to include MSETNX et al.): https://microsoft.github.io/garnet/docs/welcome/compatibility (see point #1).

We may consider adding an optional strict mode to Garnet that would run this as a transaction, so only those who require the stricter semantics take up the added cost of the transaction.

@prvyk
Copy link
Contributor Author

prvyk commented Jan 31, 2025

Yeah, properly supporting requires the transactional semantics, otherwise you get inevitable race conditions (the current implementation is basically a foreach SETNX. One could have a variable set between the SET_Conditional calls after the MSETNX call was issued). The question is which failure mode is more acceptable when not running in 'strict mode'. I suspect 'working per the docs (except the race)' would be more aligned to user expectations?

@badrishc
Copy link
Contributor

badrishc commented Feb 2, 2025

It seems the use cases of MSETNX such as distributed locks indeed depend on an atomic set keys only if none of the keys exist. For such a use case to work correctly, MSETNX will have to be implemented as a proper transaction.

@badrishc badrishc linked a pull request Feb 5, 2025 that will close this issue
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 a pull request may close this issue.

2 participants