Fix KV create race after delete/purge #1301
Merged
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
Jarema
requested changes
Aug 14, 2024
fnichol
added a commit
to systeminit/si
that referenced
this pull request
Aug 14, 2024
This change updates the Git branch to an `si/stable` branch so that any upstream changes can be made to the `js-kv-recreate-race` branch without impacting this codebase directly. References: nats-io/nats.rs#1301 Signed-off-by: Fletcher Nichol <[email protected]>
fnichol
force-pushed
the
js-kv-recreate-race
branch
from
5209103
to
d4432c1
Compare
Jarema
requested changes
Aug 14, 2024
This change fixes an issue when using a JetStream K/V store where a user is creating, deleting, and re-creating keys. If the last entry for a key is a `Operation::Delete` or `Operation::Purge`, the initial `self.update()` returns an error, causing the second part of the method to be exercised. Prior to this change, if the entry was deleted or purged a `kv.put()` call is used which ignores the revision of that last entry. A single writer to the K/V store would succeed (as no other writers would write first) so no problem. However, if 2 writers attempt to create a key, then a second writer *could* call the `kv.put()` before the first writer calls `kv.put()`. This means that *both* writers get an `Ok(revision)` and can assume that they won the creation of the key. When using a "distributed lock" pattern (that is many writers race to create a key and the first successful writer wins), this above scenario results in potentially more than one writer who believes they have uniquely acquired the distributed lock. This change replaces the `kv.put()` call to a `kv.update()` call and provides the `revision` from the deleted/purged entry to ensure that no other writer has beaten the caller to this update. This change closes the race period between concurrent writers to between the first update and the second update call with some optimistic write concurrency to detect another writer. It appears as though this strategy is in effect in the Go client code [kv.Create] implementation. [kv.Create]: https://github.com/nats-io/nats.go/blob/278f9f188bca4d7bdee283a0e98ab66b82530c60/jetstream/kv.go#L944-L963 Co-authored-by: John Keiser <[email protected]> Signed-off-by: Fletcher Nichol <[email protected]> Signed-off-by: Fletcher Nichol <[email protected]>
fnichol
force-pushed
the
js-kv-recreate-race
branch
from
d4432c1
to
4e8c526
Compare
Jarema
approved these changes
Aug 15, 2024
fnichol
added a commit
to systeminit/si
that referenced
this pull request
Sep 12, 2024
Notable is that this release contains our upstream KV create race fix and some lower level data types to work with direct getting messages. https://github.com/nats-io/nats.rs/releases/tag/async-nats%2Fv0.36.0 References: nats-io/nats.rs#1301 Signed-off-by: Fletcher Nichol <[email protected]>
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.
This change fixes an issue when using a JetStream K/V store where a user is creating, deleting, and re-creating keys. If the last entry for a key is a
Operation::DeleteorOperation::Purge, the initialself.update()returns an error, causing the second part of the method to be exercised.Prior to this change, if the entry was deleted or purged a
kv.put()call is used which ignores the revision of that last entry. A single writer to the K/V store would succeed (as no other writers would write first) so no problem. However, if 2 writers attempt to create a key, then a second writer could call thekv.put()before the first writer callskv.put(). This means that both writers get anOk(revision)and can assume that they won the creation of the key.When using a "distributed lock" pattern (that is many writers race to create a key and the first successful writer wins), this above scenario results in potentially more than one writer who believes they have uniquely acquired the distributed lock.
This change replaces the
kv.put()call to akv.update()call and provides therevisionfrom the deleted/purged entry to ensure that no other writer has beaten the caller to this update. This change closes the race period between concurrent writers to between the first update and the second update call with some optimistic write concurrency to detect another writer.It appears as though this strategy is in effect in the Go client code kv.Create implementation.