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.
There was a problem hiding this comment.
I usually put context comment like this in commit message/PR description and as github comments. Github comments have the highest chance of being noticed by other reviewers, but unfortunately don't get saved in git history 😞
| # 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?
There was a problem hiding this comment.
We need to serialize subscription and calls to "immediate handler" with respect to events fired by the provider. So updates to client status, firing of immediate handler, and provider event handlers must be synchronized in some way — either a lock or some kind of singe-threaded queue. Not sure if the client lock here is enough — need to do a deeper dive into the code
5 participants
This PR
Related Issues
#96
Notes