fix: thread safety issues#614
Conversation
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
+ Coverage 98.34% 98.36% +0.02%
==========================================
Files 45 45
Lines 2483 2514 +31
==========================================
+ Hits 2442 2473 +31
Misses 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR hardens shared-state access across the Python SDK: hook lists (global and per-client) now mutate under ChangesShared-state hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
leakonvalinka
changed the title
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
leakonvalinka
force-pushed
the
fix/thread-safety-adjustments
branch
from
0c9eb51 to
5d11dbc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openfeature/provider/_registry.py (1)
99-105: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winAvoid clearing event handlers while holding the registry lock.
clear_event_handlers()acquires_client_lock; concurrently, handler dispatch holds_client_lockwhile resolvingclient.provider, which now acquiresProviderRegistry._lock. This creates a registry-lock → client-lock path here and a client-lock → registry-lock path during dispatch, soclear_providers()can deadlock with provider event dispatch.🔒 Proposed fix
def clear_providers(self) -> None: self.shutdown() with self._lock: self._providers.clear() self._default_provider = NoOpProvider() self._provider_status = { self._default_provider: ProviderStatus.READY, } - clear_event_handlers() + + clear_event_handlers()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openfeature/provider/_registry.py` around lines 99 - 105, clear_providers() is calling clear_event_handlers() while still holding ProviderRegistry._lock, which creates a lock-order cycle with event dispatch. Move the clear_event_handlers() call out of the locked section in ProviderRegistry.clear_providers(), keeping only provider state updates under the lock and then clearing handlers after the lock is released. Use the ProviderRegistry._lock, clear_providers(), and clear_event_handlers() symbols to update the flow so the registry lock is never held when acquiring the client/event-handler lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openfeature/transaction_context/__init__.py`:
- Around line 26-50: The transaction-context helpers can self-deadlock because
_propagator_lock is a non-reentrant Lock while get_transaction_context and
set_transaction_context invoke user-provided TransactionContextPropagator
methods under that lock. Update the locking strategy in
openfeature/transaction_context/__init__.py so re-entrant calls from custom
propagators do not block the same thread, for example by switching
_propagator_lock to an RLock and keeping the existing serialization around
set_transaction_context_propagator, get_transaction_context, and
set_transaction_context.
---
Outside diff comments:
In `@openfeature/provider/_registry.py`:
- Around line 99-105: clear_providers() is calling clear_event_handlers() while
still holding ProviderRegistry._lock, which creates a lock-order cycle with
event dispatch. Move the clear_event_handlers() call out of the locked section
in ProviderRegistry.clear_providers(), keeping only provider state updates under
the lock and then clearing handlers after the lock is released. Use the
ProviderRegistry._lock, clear_providers(), and clear_event_handlers() symbols to
update the flow so the registry lock is never held when acquiring the
client/event-handler lock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e01e6e3-2b84-4dab-a926-10ec95fae6b5
📒 Files selected for processing (8)
openfeature/_event_support.pyopenfeature/api.pyopenfeature/client.pyopenfeature/hook/__init__.pyopenfeature/provider/__init__.pyopenfeature/provider/_registry.pyopenfeature/transaction_context/__init__.pytests/test_client.py
💤 Files with no reviewable changes (1)
- openfeature/api.py
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
leakonvalinka
force-pushed
the
fix/thread-safety-adjustments
branch
from
7f14e05 to
22d9583
Compare
|
I am curious, can we add a tool to detect this like vmlens in java? In the sense of automated testing? |
gruebel
left a comment
gruebel
left a comment
There was a problem hiding this comment.
thanks 🍻 overall it looks pretty good and adds more thread safety especially when using free-threaded CPython
| if hasattr(self, "_on_emit"): | ||
| self._on_emit(self, event, details) | ||
| on_emit = getattr(self, "_on_emit", None) | ||
| if on_emit is not None: | ||
| on_emit(self, event, details) |
There was a problem hiding this comment.
because of what was pointed out in the original issue:
"
AbstractProvider._on_emit (provider/__init__.py) - emit() does if hasattr(self, "_on_emit"): self._on_emit(...) which is a TOCTOU; detach() during shutdown can delete _on_emit while a background thread is between the check and the call
"
|
|
||
| def get_transaction_context() -> EvaluationContext: | ||
| return _evaluation_transaction_context_propagator.get_transaction_context() | ||
| with _propagator_lock: |
There was a problem hiding this comment.
not sure about this one here, if this is really needed or not
There was a problem hiding this comment.
Same here, I tried to ask Claude but the answer was sometimes yes and sometimes no 😅
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
|
@aepfli I don't think so, unfortunetly. I did try to write a few tests and also have Claude write some with |
toddbaert
requested review from
federicobond,
gruebel,
kinyoklion,
lukas-reining,
matthewelwell and
nicklasl
| def get_transaction_context() -> EvaluationContext: | ||
| return _evaluation_transaction_context_propagator.get_transaction_context() | ||
| with _propagator_lock: | ||
| propagator = _evaluation_transaction_context_propagator | ||
| return propagator.get_transaction_context() | ||
|
|
||
|
|
||
| def set_transaction_context(evaluation_context: EvaluationContext) -> None: | ||
| global _evaluation_transaction_context_propagator | ||
| _evaluation_transaction_context_propagator.set_transaction_context( | ||
| evaluation_context | ||
| ) | ||
| with _propagator_lock: | ||
| propagator = _evaluation_transaction_context_propagator | ||
| propagator.set_transaction_context(evaluation_context) |
There was a problem hiding this comment.
We're holding _propagator_lock across the propagator's get and set, but I think only the reference read needs guarding; the swap in set_transaction_context_propagator is the sole writer, and reading the global is atomic anyway, if I'm understanding the Python docs right...
Holding it through the call serializes all transaction-context access across threads, and might be problematic especially if somebody was doing a lot in a custom propagator.
| def get_transaction_context() -> EvaluationContext: | |
| return _evaluation_transaction_context_propagator.get_transaction_context() | |
| with _propagator_lock: | |
| propagator = _evaluation_transaction_context_propagator | |
| return propagator.get_transaction_context() | |
| def set_transaction_context(evaluation_context: EvaluationContext) -> None: | |
| global _evaluation_transaction_context_propagator | |
| _evaluation_transaction_context_propagator.set_transaction_context( | |
| evaluation_context | |
| ) | |
| with _propagator_lock: | |
| propagator = _evaluation_transaction_context_propagator | |
| propagator.set_transaction_context(evaluation_context) | |
| def get_transaction_context() -> EvaluationContext: | |
| with _propagator_lock: | |
| propagator = _evaluation_transaction_context_propagator | |
| return propagator.get_transaction_context() | |
| def set_transaction_context(evaluation_context: EvaluationContext) -> None: | |
| with _propagator_lock: | |
| propagator = _evaluation_transaction_context_propagator | |
| propagator.set_transaction_context(evaluation_context) |
There was a problem hiding this comment.
I can change it, I'm really not sure myself..
| # while the lock guarantees safety, even without it there was never a loss within 50.000 runs (with the default GIL | ||
| # switch interval of 5ms). only when the switch interval was significantly shortened to 0.1 microseconds, losses were | ||
| # observed without locks every now and then. with a no-GIL python, the lock would be essential |
There was a problem hiding this comment.
minor: this comment seems trying to justify that it's "fine" to not have a lock in some cases?
We know the bug is here, and the first rule of testings tells us that we can't prove the absence of bugs with tests — only their presence. Longer switch interval doesn't make it safe — just make less likely to be observed. And as one person put it: when you serve millions of users, once-in-a-million events happen every day
Anyway, this comment doesn't look like it belongs to the code — once the lock is here, there's not much point in debating/defending it
There was a problem hiding this comment.
Agree, I would also remove this comment before merging the PR. I added it to give people some context and share my findings, as personally I wouldn't have known this by just looking at the changed code. As I am not that well-versed in Python to be able to tell whether this is a basically-impossible issue or how expensive an unnecessary lock would be, I thought maybe someone else could share their knowledge here.
| # Guards the read-concat-store against a lost update; this practically never races under the default 5ms GIL | ||
| # switch interval, but is essential under a no-GIL build. |
There was a problem hiding this comment.
minor: misleading comment again. It's not "never" — just rarely
|
|
||
| def clear() -> None: | ||
| with _global_lock: | ||
| with _global_lock, _client_lock: |
There was a problem hiding this comment.
minor: this increases the length of critical sections, increasing the change of contention. Not that it's called too frequently but it's moving in the opposite direction
There was a problem hiding this comment.
Fair point, I changed it because it was a mentioned issue in #96, but I guess not changing it makes more sense..?
| # outside the lock intentionally: the immediate-fire status check acquires the registry lock, so calling it | ||
| # under _client_lock risks lock-order inversion against run_handlers_for_provider (registry lock → _client_lock). | ||
| # As a consequence, a narrow double-fire is possible: if dispatch_event(client's event) runs concurrently, it | ||
| # sets the matching provider status (enabling the immediate fire below) and then re-runs every handler for this | ||
| # client. If _run_immediate_handler lands after that status set but before dispatch snapshots the handler list, | ||
| # the handler fires twice — once here, once from dispatch. Only happens when the registered event matches the event | ||
| # being dispatched; otherwise the immediate fire is a no-op. |
There was a problem hiding this comment.
minor: this comment says that we're trading one concurrency issue for another — not a good place to be in. _run_immediate_handler is not synchronized with run_handlers_for_provider, so a provider event may race with the immediate handler and the user may receive events in reverse order. We'd need to do something about it
There was a problem hiding this comment.
Ah I'm sorry, this comment is outdated now that I have removed the locks for the get methods in _registry.py. However, after taking another look I don't think the double-fire possibility is entirely eliminated either way - even if _run_immediate_handler was moved inside the lock here. I will therefore adjust the comment, should I leave the method call outside the lock or move it inside?
5 participants
This PR
Related Issues
#96
Notes