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.37% +0.02%
==========================================
Files 45 45
Lines 2483 2518 +35
==========================================
+ Hits 2442 2477 +35
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds locking around shared hook, provider, and transaction-context state, moves event-handler cleanup into the provider registry, and changes client provider-status checks to use the resolved provider with test coverage updates. ChangesShared-state hardening
Transaction context locking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
0c9eb51 to
5d11dbc
Compare
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>
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? |
| # to be shut down isn't returned | ||
| # however it contributes to a potential deadlock that is currently | ||
| # still in place (clear_providers: registry's lock -> _event_support's lock; | ||
| # run_handlers_for_provider: _event_support's lock -> registry's lock) |
There was a problem hiding this comment.
just writing a comment here as a TODO that still needs to be resolved before merging
This PR
Related Issues
#96
Notes