feat: provider wrapper to support state race fix#487
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #487 +/- ##
==========================================
- Coverage 84.07% 83.88% -0.19%
==========================================
Files 27 27
Lines 2079 2098 +19
==========================================
+ Hits 1748 1760 +12
- Misses 292 295 +3
- Partials 39 43 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
poc/fix-provider-init-race
branch
from
65b6454 to
a87f422
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the StateManagingProvider interface, allowing providers to manage their own state and event emissions, thereby decoupling them from the SDK's shadow state management. The changes include updates to eventExecutor and evaluationAPI to respect this interface, as well as caching the capability in providerReference. I have provided feedback regarding the optimization of type assertions by utilizing the new delegateManagesState field and a recommendation to avoid holding the executor mutex during external provider calls to prevent potential deadlocks.
| if smp, ok := providerReference.featureProvider.(StateManagingProvider); ok { | ||
| state = smp.State() | ||
| } else { |
There was a problem hiding this comment.
Since providerReference now includes the delegateManagesState flag, we can use it here to avoid the type assertion check, improving clarity and consistency with the new field.
| if smp, ok := providerReference.featureProvider.(StateManagingProvider); ok { | |
| state = smp.State() | |
| } else { | |
| if providerReference.delegateManagesState { | |
| state = providerReference.featureProvider.(StateManagingProvider).State() | |
| } else { |
| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
|
|
||
| // find the provider reference for this domain | ||
| ref, ok := e.namedProviderReference[domain] | ||
| if !ok { | ||
| ref = e.defaultProviderReference | ||
| } | ||
|
|
||
| // state-managing providers own their state; read directly | ||
| if smp, ok := ref.featureProvider.(StateManagingProvider); ok { | ||
| return smp.State() | ||
| } |
There was a problem hiding this comment.
Holding the executor's mutex while calling smp.State() is risky as it involves calling into external provider code while holding a lock. This can lead to performance bottlenecks or potential deadlocks if the provider's State() method blocks or attempts to call back into the SDK. Since we only need the lock to retrieve the provider reference, we can release it early. Additionally, we can leverage the delegateManagesState flag.
e.mu.Lock()
// find the provider reference for this domain
ref, ok := e.namedProviderReference[domain]
if !ok {
ref = e.defaultProviderReference
}
e.mu.Unlock()
// state-managing providers own their state; read directly
if ref.delegateManagesState {
return ref.featureProvider.(StateManagingProvider).State()
}| e.mu.Lock() | ||
| defer e.mu.Unlock() | ||
|
|
||
| _, delegateManagesState := handler.(StateManagingProvider) |
There was a problem hiding this comment.
This type assertion is redundant because the providerReference already caches this information in the delegateManagesState field. You can remove this line and use the flag from the references in the subsequent logic (e.g., reference.delegateManagesState and e.defaultProviderReference.delegateManagesState).
1 participant
PoC for open-feature/spec#365. This adds a
StateManagingProviderinterface (opt-in) so providers can own their state and event emissions, fixing the race between SDK-managed state writes and provider-emitted events during/after init. When a provider implements this interface, the SDK reads state directly from the provider and skips its own state writes and event emissions in the init, shutdown, and runtime event paths. Legacy providers that do not implement the interface continue to work exactly as before (SDK manages their state). Not breaking for application-authors or provider-authors.Companion PoCs: