Store: serialize all operations with an internal lock, fixes #206 - #207
Merged
Conversation
A Store instance can now be shared between threads. Neither the backends (e.g. one paramiko sftp session, one requests session for rest) nor the Store's own bookkeeping (the _stats Counter, the writethrough cache accounting) are safe under concurrent calls, so all operations now take a store-level RLock (reentrant, because operations nest: create_levels uses "with self:", load/store/... call find). list() stays a lazy generator: the lock is only held while fetching the next item, not across the whole iteration, so other threads' operations interleave with a long listing and the iterating thread itself can do store operations inside its listing loop without deadlocking. Serialization is per operation: multi-operation sequences that need to be atomic against other threads must still be coordinated by the caller. The uncontended lock costs ~100ns per operation, noise compared to any backend call, so the locking is unconditional rather than opt-in. This is needed by borgbackup/borg#9988: borg2's PackWriter gets a background store-thread that stores full packs while the main thread assembles the next pack - and keeps using the same Store (lock refresh, reads of already stored packs, index writes) in the meantime. Tests: a serialization-asserting backend wrapper (fails when two backend calls overlap - verified to trigger without the lock), a list() laziness / interleaving test, and a stats lost-update test.
This file contains hidden or 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
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.
Fixes #206.
A
Storeinstance can now be shared between threads: all operations are serialized by an internalRLock(reentrant, because operations nest:create_levelsuseswith self:,load/store/… callfind). This protects both the backends (one paramiko sftp session / one requests session are not thread-safe) and the Store's own bookkeeping (_statsCounter read-modify-writes, writethrough cache accounting).Design points, as discussed in #206:
list()stays a lazy generator: the lock is only held while fetching the next item, not across the whole iteration — so other threads' operations interleave with a long listing, and the iterating thread itself can do store operations inside its listing loop without deadlocking on the lock.threading.RLockcosts ~100 ns per acquire, noise next to any backend call (even posixfs).__init__because some@_lockedmethods (set_levels) already run during construction.Tests (
tests/test_threading.py):SerializationAssertingBackend: wraps posixfs, flags any overlapping backend calls (with a small sleep to widen the race window). Verified that the hammer test does trigger violations when the lock is neutered, so the test is load-bearing.list()laziness: same-thread ops inside the listing loop + another thread's ops interleaving with a long listing.Motivation: borgbackup/borg#9988 — borg2's
PackWritergets a background store-thread that stores full packs while the main thread assembles the next pack and keeps using the sameStore(lock refresh, reads of already stored packs, index writes). With this released, borg can drop its interimSerializedStorewrapper and just bump its borgstore pin.(implemented by Claude, reviewed by TW)
🤖 Generated with Claude Code