feat: add CacheLock and the expire_if_equals backend primitive - #150
Conversation
…ext manager (allen0099#62) - Implement expire_if_equals(key, expected, ttl) atomic primitive on BaseCacheBackend, MemoryBackend, AsyncRedisCacheBackend (Lua script), and MemcachedBackend (CAS). - Document expire_if_equals in docs/BACKENDS.md under 'Atomic backend primitives'. - Implement CacheLock distributed lock helper and async context manager in fastapi_cachex/lock.py. - Add LockTimeoutError exception raised on context manager timeout. - Export CacheLock and LockTimeoutError from package root. - Add comprehensive unit test suite in tests/test_lock.py and tests/backends/. - Add CHANGELOG entry under [Unreleased].
|
Hi @allen0099, |
allen0099
left a comment
There was a problem hiding this comment.
Thanks, @ShivanshShukla, this is a solid first cut, and it follows the design we agreed on in #64 closely. acquire() returns False and only __aenter__ raises LockTimeoutError, a lost release() stays quiet, and the Redis and Memcached expire_if_equals implementations mirror delete_if_equals, including the race tests. A few things before this can leave draft:
Scope and linking
- Let's keep this as one PR rather than splitting it. Both halves are already here and belong together. Please add
Closes #64to the description so the issue closes on merge. #62 is the earlier, already closed primitive issue.
Code
-
fastapi_cachex/lock.pyimportsSelffromtyping_extensions, which is not a declared dependency of this package (it only arrives through pydantic). Please return"CacheLock"from__aenter__instead, as a quoted annotation, the way the rest of the package handles forward references. -
LOCK_FINGERPRINTandlock_entry()intypes.pybecome public API, but onlylock.pyuses them. Please move them intolock.pyas private helpers (_LOCK_FINGERPRINT,_lock_entry). -
acquire(timeout=None)means "use the instance default", so a caller can't ask for an unbounded wait on one call when the instance has a timeout. That's acceptable, but please say so in theacquiredocstring. -
Blocking: the token belongs to the instance, so two acquisitions through the same instance share it, and that silently defeats the owner check. Two ways to get there:
- Reentry. Calling
acquire()again on an instance that already holds the lock polls until the lock's own TTL expires. It then re-acquires with the same token while the outer critical section is still running. When the inner block releases,delete_if_equalsmatches and frees the lock under the outer block, so another process can take it while the outer code still runs. Nothing is raised or logged. - A shared instance. This one is more likely, because it is how people use
asyncio.Lock: a module-levelreport_lock = CacheLock("report")used withasync with report_lock:in concurrent requests. Once one request overruns the TTL and another takes the lock, the first request'srelease()deletes the second request's lock. That is exactly the racedelete_if_equalsexists to prevent.
Documentation alone won't stop either of these. Please track holding state on the instance and make
acquire()raiseRuntimeErrorwhen the instance already holds the lock. The message should tell the caller to use oneCacheLockper acquisition. Clear the state onrelease()(successful or not). Please don't returnFalsehere:Falsemeans "someone else holds it", and underasync withit would surface as a misleadingLockTimeoutError. Also mention the one-instance-per-acquisition rule in the class docstring, and add tests for both the reentrant and the shared-instance case. - Reentry. Calling
Tests
CacheLockitself is only exercised againstMemoryBackend. Please add at least one acquire → extend → release round trip against Redis and Memcached (requires_redis/requires_memcached), since that is where the lock matters.- Nit: move the
import asynciointest_lock_acquire_blocking_indefinite_retries_until_availableto the top of the module. - CI is green on your side: all tests pass on 3.10–3.14 and in tox, and coverage stays at 100% with live Redis and Memcached. The red "coverage" check comes from its badge-upload step, which tries to push to this repo and can't from a fork. That's a bug in our workflow, not in your PR, and I'll fix it separately. Please keep coverage at 100% as you add the changes above.
Docs
CacheLockhas no user-facing docs yet. Please add:- a new guide,
docs/LOCK.md, added to the nav inzensical.tomlnext to the other guides. It should cover the two usage patterns from #64, blocking vs. non-blocking acquire and timeouts, renewing withextend(), what happens when the TTL runs out beforerelease(), and the one-instance-per-acquisition rule from point 5. Please also add it to the documentation list inREADME.md; - an API reference entry, e.g.
::: fastapi_cachex.lock.CacheLockin a newdocs/api/lock.mdadded to the nav inzensical.toml.LockTimeoutErroralready appears indocs/api/types.md, which documents the whole exceptions module.
- a new guide,
- Please link #64 in the CHANGELOG entries (
([#64](https://github.com/allen0099/FastAPI-CacheX/issues/64))), as the other entries do.
You don't need to touch the Traditional Chinese docs under i18n/. Those get translated separately.
Thanks again!
The Coverage Badge workflow runs on pull requests too, and its last step pushed the badge to the coverage-badge branch every time. A pull request from a branch in this repository overwrote master's badge with the pull request's coverage. A pull request from a fork gets a read-only token, so the push failed with 403 and the check went red even though tests and coverage passed (seen on #150). Run the upload step only for push events. Pull requests still run the suite against live servers and enforce the coverage gate.
) - Move _LOCK_FINGERPRINT and _lock_entry into lock.py as private helpers. - Add _is_held state guard to CacheLock to prevent re-entry or instance sharing across concurrent tasks. - Remove Self import from typing_extensions; use string return annotation for __aenter__. - Update acquire() docstring timeout parameter description. - Add RuntimeError re-entry and shared instance unit tests in tests/test_lock.py. - Add live Redis and Memcached integration tests for CacheLock. - Add docs/LOCK.md guide, docs/api/lock.md API reference, and update navigation in zensical.toml, zensical.zh-TW.toml, and README.md. - Update CHANGELOG.md entry with issue link allen0099#64.
|
@allen0099 Thanks for the thorough review! The catch on the shared instance token overlap was spot on. I've pushed a new commit addressing everything: |
allen0099
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, @ShivanshShukla. Most of the first review is addressed: the private helpers, the quoted __aenter__ annotation, the acquire docstring, the live Redis and Memcached round trips, and the new guide and API page all look good. A few things are still left:
Blocking
-
The shared-instance check can still be bypassed.
acquire()only sets_is_heldafterset_if_absentsucceeds, and there is anawaitin between. Two tasks that callacquire()on the same instance while it is free both get past the check. One of them wins, and the other keeps polling with the same token. If the winner overruns the TTL, the loser takes the lock with that token, and the winner'srelease()deletes it: the original race.test_lock_shared_instance_raises_runtime_errorpasses only becauseMemoryBackend.set_if_absentdoesn't yield when its lock is free. Over Redis or Memcached, where every call really awaits, both tasks pass the check.I reproduced it against a live Redis with one shared
CacheLock("shared", ttl=1): task A holds it for 1.5 s, task B callsacquire()at the same time, and a separateCacheLock("shared")tries a non-blocking acquire while B is inside. Nothing raises, and the log is:A acquired B acquired # while A is still inside A released -> True # deletes the lock B now holds outsider acquired -> True # while B is still inside B released -> FalsePlease mark the instance as in use at the top of
acquire(), before the firstawait, and clear the mark if the acquisition fails or times out (and inrelease(), as now). A secondacquire()on the instance then raisesRuntimeErrorright away, whatever the backend does. For the test, please use a backend whoseset_if_absentyields (for example, aMemoryBackendsubclass that doesawait asyncio.sleep(0)first), or run the shared-instance case against Redis as well, so the test fails without the fix. -
CHANGELOG format. #153 has just been merged: every changelog entry now has to open with a bold one-line summary, because the GitHub release notes are built from those summaries. Your merge from
masteralready brought it in, so CI will fail on this branch as it stands (test_the_repository_changelog_can_be_released). Please reword both entries along these lines, link #64 on theexpire_if_equalsentry too, and leave a blank line before### Changed:### Added - **`CacheLock`, a distributed lock built on the backend primitives.** ...details... ([#64](https://github.com/allen0099/FastAPI-CacheX/issues/64)) - **`expire_if_equals()` backend primitive for owner-checked TTL renewal.** ...details... ([#64](https://github.com/allen0099/FastAPI-CacheX/issues/64))
See "The changelog is part of the release now" in
docs/DEVELOPMENT.md.
Docs
- Please revert the change to
zensical.zh-TW.toml. The Traditional Chinese site has noLOCK.md, so the new nav entry links to a 404 in the preview (https://fastapi-cachex--150.org.readthedocs.build/zh-tw/150/LOCK/). The page gets added there when it is translated. docs/LOCK.mddoesn't yet cover what happens when the TTL runs out beforerelease(). Please say that plainly: the lock becomes free, another process can take it while your code is still running, andextend()/release()then returnFalse. Advise choosing a TTL longer than the work, or callingextend()periodically. Please also say that the defaulttimeout=Nonemakes a blockingacquire()(andasync with) wait indefinitely.
Minor
- Instead of a file-wide
PYI034ignore inpyproject.toml, please use# noqa: PYI034on the__aenter__line, so the rule stays active for the rest of the module. - The PR description still says it addresses #62, and it doesn't have
Closes #64yet. Please update it so the issue closes on merge.
Thanks!
…len0099#64) - Set _is_held = True synchronously at entry of acquire() before the first await to prevent shared-instance race conditions. - Format CHANGELOG.md entries with bold summaries and link allen0099#64 on both. - Revert zensical.zh-TW.toml edit. - Update docs/LOCK.md to detail TTL expiration behavior and default timeout=None indefinite blocking. - Move PYI034 ignore inline on __aenter__ line in lock.py. - Update test_lock_shared_instance_raises_runtime_error in tests/test_lock.py to use YieldingMemoryBackend.
allen0099
left a comment
There was a problem hiding this comment.
Thanks, @ShivanshShukla, this round looks good. I re-ran my shared-instance reproduction against a live Redis: the second acquire() now raises RuntimeError straight away, and the race is gone. The CHANGELOG entries, the LOCK.md additions, the inline noqa and the zensical.zh-TW.toml revert are all as asked. One thing left:
A cancelled acquire() leaves the instance stuck. acquire() clears _is_held in except Exception, but asyncio.CancelledError is a BaseException, not an Exception. So a blocking acquire() that is cancelled, for example by asyncio.wait_for()/asyncio.timeout() or a cancelled request, leaves _is_held = True, and every later acquire() on that instance raises RuntimeError even though it never got the lock:
waiter = CacheLock("job", ttl=30) # "job" is held elsewhere
try:
await asyncio.wait_for(waiter.acquire(), timeout=0.3)
except asyncio.TimeoutError:
pass
waiter._is_held # True
await waiter.acquire(blocking=False) # RuntimeError: ... already heldPlease change it to except BaseException: (it re-raises, so nothing is swallowed), and add a test that cancels a blocking acquire() and then acquires again on the same instance.
I've updated the PR title (it was cut off) and the description so that it says Closes #64, which will close the issue on merge. No action needed there.
After that, please mark the PR ready for review.
|
One more thing, which CI found once it ran: The simplest fix is to move the two tests into |
|
Thanks for catching that, @allen0099!
|
Lint failed on ruff format --check for the two CacheLock lifecycle tests moved into tests/backends/.
|
Thanks, the fixture fix works: with live servers in CI, both lifecycle tests now pass. Lint failed only because One point from my review just before the fixture comment is still open: |
|
Thanks @allen0099! Pulled
All 626 tests, |
|
Thanks, the cancellation fix and its test look good. I checked that the new test fails if
From my side this is ready. Please mark it ready for review. |
Summary
Closes #64. The
expire_if_equalsprimitive was tracked in #62.This PR introduces:
expire_if_equals) across all backends.CacheLockhelper usable as anasynccontext manager or via explicitacquire/release/extend/lockedcalls.Proposed Changes
1.
expire_if_equals(key, expected, ttl) -> boolPrimitiveBaseCacheBackend: Added non-abstract method with non-atomic fallback andvalidate_ttl(ttl).MemoryBackend: Overridden underself.lock.AsyncRedisCacheBackend: Overridden using Lua script_EXPIRE_IF_EQUALS_SCRIPT(GETcompare +EXPIRE).MemcachedBackend: Overridden usingGETS+CASwrite with the new exptime (TOUCHtakes no CAS token).docs/BACKENDS.md: Documentedexpire_if_equalsunder the Atomic backend primitives section.2.
CacheLockDistributed Lock & HelpersCacheLock(fastapi_cachex/lock.py):acquire(blocking=True, timeout=None, poll_interval=0.1, ttl=None) -> bool: non-blocking mode uses singleset_if_absent; blocking mode retries with deadline loop untiltimeout.release() -> bool: callsdelete_if_equals. Silently returnsFalseif lock was lost or expired.extend(ttl=None) -> bool: callsexpire_if_equalsto safely renew holder's lease.locked() -> bool: queries whether lock key exists in backend.key_prefix="lock:"(defaults underlock:namespace).backend=parameter defaulting toBackendProxy.get().secrets.token_hex(16).LockTimeoutError(fastapi_cachex/exceptions.py): Raised by__aenter__when context managerasync with CacheLock(..., timeout=X)fails to acquire lock within timeout.CacheLockandLockTimeoutErrorfromfastapi_cachexroot.CHANGELOG.md: Added release notes under[Unreleased].Usage Example