Skip to content

Commit be27f1b

Browse files
committed
fix(circuit_breaker): address review feedback
- Make the counter race test catch a missing lock: park readers mid-increment so an unsynchronized read-modify-write deterministically loses updates (fails 10/10 unlocked, passes 10/10 locked) - Document that on_circuit_open and on_transition callbacks can run concurrently and must be thread-safe - Use dict.get's default instead of `or` for the per-circuit settings lookup
1 parent 9f547d8 commit be27f1b

3 files changed

Lines changed: 26 additions & 4 deletions

File tree

‎aws_lambda_powertools/utilities/circuit_breaker_alpha/persistence/base.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ def configure(self, config: CircuitBreakerConfig, circuit_name: str) -> None:
100100
def _settings_for(self, name: str) -> _CircuitSettings:
101101
"""Return a circuit's configured settings, or the defaults if never configured."""
102102
with self._lock:
103-
return self._settings.get(name) or _DEFAULT_SETTINGS
103+
return self._settings.get(name, _DEFAULT_SETTINGS)
104104

105105
# ------------------------------------------------------------------ cache
106106

‎docs/utilities/circuit_breaker.md‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,13 +186,16 @@ After `recovery_timeout` seconds, the circuit moves to `HALF_OPEN` and elects a
186186
!!! note "Thread safety"
187187
The utility is safe to share across threads: within a multi-threaded environment the probe election picks a single
188188
thread, so the single-prober guarantee spans threads as well as environments, and the in-memory failure counter is
189-
synchronized. Single-threaded functions (the normal Lambda model) are unaffected. `on_circuit_open` and
190-
`on_transition` hooks may run concurrently from multiple threads.
189+
synchronized. Single-threaded functions (the normal Lambda model) are unaffected.
191190

192191
Probe ownership belongs to the thread that won the election. If that thread never runs the circuit again (for
193192
example, a thread-per-request worker pool), recovery waits for the probe lease to expire before another thread or
194193
environment takes over.
195194

195+
!!! warning "Make your hooks thread-safe"
196+
If your function runs multiple threads, `on_circuit_open` and `on_transition` callbacks can run concurrently
197+
for the same circuit. Make them thread-safe.
198+
196199
### State coordination across environments
197200

198201
The consecutive-failure counter lives in memory per execution environment, so a healthy circuit performs **no writes**. Only when an environment reaches `failure_threshold` does it persist `OPEN`. The shared state is cached locally for `local_cache_max_age` seconds to avoid a read per invocation. A cache miss (cold start or expired entry) forces a read-through before routing.

‎tests/functional/circuit_breaker_alpha/test_circuit_breaker.py‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
from __future__ import annotations
22

33
import threading
4+
import time
45
import warnings
56

67
import pytest
@@ -680,8 +681,26 @@ def worker():
680681
assert store.db["c"].half_open_owner is not None
681682

682683

683-
def test_threads_trip_at_exactly_the_failure_threshold(store):
684+
class _RacyCounters(dict):
685+
"""Counter dict that yields between the read and the write of an increment.
686+
687+
Without this, the GIL makes the unlocked read-modify-write effectively atomic
688+
(the interpreter rarely switches threads inside those few bytecodes), so a
689+
missing lock would still pass. Holding every reader mid-increment long enough
690+
for its siblings to read the same stale value makes the lost update
691+
deterministic: unlocked, all threads write back the same count and the
692+
threshold is never reached.
693+
"""
694+
695+
def get(self, key, default=None):
696+
value = super().get(key, default)
697+
time.sleep(0.005) # park mid-increment so sibling threads read the same stale value
698+
return value
699+
700+
701+
def test_threads_trip_at_exactly_the_failure_threshold(store, monkeypatch):
684702
"""Racing increments must neither lose updates (tripping late) nor persist the trip twice."""
703+
monkeypatch.setattr(base_module, "_LOCAL_FAILURES", _RacyCounters())
685704
threads_count, threshold = 8, 5
686705
config = CircuitBreakerConfig(failure_threshold=threshold, local_cache_max_age=0)
687706
barrier = threading.Barrier(threads_count)

0 commit comments

Comments
 (0)