fix: provider state race #380
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| --- | ||
| id: appendix-e | ||
| title: "Appendix E: Migrations" | ||
| description: Migration guidance for breaking spec changes | ||
| sidebar_position: 6 | ||
| --- | ||
|
|
||
| # Appendix E: Migrations | ||
|
|
||
| This appendix provides non-normative guidance for provider authors and SDK authors on migrating to new or changed specification requirements. | ||
|
|
||
| ## Provider Status Ownership | ||
|
|
||
| ### Background | ||
|
|
||
| Prior to `v0.9.0`, provider status (e.g. `NOT_READY`, `READY`, `ERROR`) was managed by the SDK on behalf of the provider. | ||
| The SDK would set status and emit events after lifecycle methods (`initialize`, `shutdown`, `on context change`) returned. | ||
| This created a race condition in multi-threaded SDKs: the provider could change its own state (e.g. emit an error event from a background thread) in the window between the lifecycle method returning and the SDK writing its post-lifecycle status and emitting the corresponding event. | ||
| The result was incorrect event ordering and inconsistent status. | ||
|
|
||
| The spec now requires providers to own their status and emit events atomically with status transitions (see [provider status](./sections/02-providers.md#28-provider-status)). | ||
|
|
||
| ### For provider authors | ||
|
|
||
| Providers are now responsible for maintaining their own `status` and emitting events atomically with status transitions. | ||
|
|
||
| #### What to implement | ||
|
|
||
| - A `status` accessor returning the provider's current readiness: `NOT_READY`, `READY`, `STALE`, `ERROR`, or `FATAL` (plus `RECONCILING` in the static-context paradigm) | ||
| - `status` must be `NOT_READY` before `initialize` is called and after `shutdown` terminates | ||
| - `status` must be safe for concurrent access | ||
| - Status transitions and associated event emissions must be atomic from the perspective of external observers; set the status before emitting the corresponding event | ||
|
|
||
| #### The `StateManagingProvider` interface | ||
|
|
||
| To signal to the SDK that your provider manages its own status, implement an opt-in interface (or equivalent mechanism) defined by the SDK. | ||
| This interface should expose: | ||
|
|
||
| - A `status` accessor that returns the provider's current status | ||
| - A discriminant or marker (e.g. an additional interface, a boolean property, or a type-level tag) that allows the SDK to detect at registration time that the provider manages its own state | ||
|
|
||
| Providers that do not implement this interface will continue to have their status managed by the SDK. | ||
| This legacy behavior is deprecated and will be removed in the next major version. | ||
|
|
||
| ### For SDK authors | ||
|
|
||
| SDKs must detect whether a registered provider manages its own state and branch behavior accordingly. | ||
|
|
||
| #### Detecting state-managing providers | ||
|
|
||
| At registration time, check whether the provider implements the `StateManagingProvider` interface (or equivalent). | ||
| Store this as a flag on the internal provider wrapper for use during lifecycle calls and event handling. | ||
|
|
||
| #### SDK wrapper behavior | ||
|
|
||
| SDKs typically wrap registered providers in an internal adapter (e.g. a "provider wrapper" or "state manager") that mediates lifecycle calls and event forwarding. | ||
| The wrapper should branch based on whether the registered provider implements the state-managing interface. | ||
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| A[Provider registered with SDK] --> B{Implements state-managing interface?} | ||
|
|
||
| B -- Yes --> C[SDK wrapper delegates status to provider] | ||
| C --> C1[initialize / shutdown / onContextChange: SDK skips state writes AND event emissions] | ||
| C --> C2[Provider events: SDK skips state writes] | ||
| C --> C3[status: reads from provider directly] | ||
|
|
||
| B -- No --> D[SDK wrapper manages state internally - legacy, deprecated] | ||
| D --> D1[initialize / shutdown / onContextChange: SDK sets state AND emits events after return] | ||
| D --> D2[Provider events: SDK updates state on emit] | ||
| D --> D3[status: reads from SDK wrapper] | ||
|
|
||
| C1 --> E[Provider-emitted events still propagate to registered handlers] | ||
| C2 --> E | ||
| C3 --> E | ||
| D1 --> E | ||
| D2 --> E | ||
| D3 --> E | ||
| ``` | ||
|
|
||
| #### What the SDK skips for state-managing providers | ||
|
|
||
| For providers that implement the state-managing interface, the SDK must not perform any of the following actions that it would normally perform for legacy providers: | ||
|
|
||
| - Setting status to `READY` after `initialize()` succeeds | ||
| - Setting status to `ERROR` or `FATAL` after `initialize()` fails | ||
| - Setting status to `NOT_READY` after `shutdown()` completes | ||
| - Emitting `PROVIDER_READY` or `PROVIDER_ERROR` events after `initialize()` | ||
| - Updating status when the provider emits events at runtime (the provider already set its own status atomically with the event) | ||
| - (Static-context paradigm only) Setting `RECONCILING` status, emitting `PROVIDER_RECONCILING`, setting `READY`/`ERROR` status, or emitting `PROVIDER_CONTEXT_CHANGED`/`PROVIDER_ERROR` during `on context change` handling | ||
|
|
||
| #### What the SDK still does for all providers | ||
|
|
||
| Regardless of whether the provider manages its own state, the SDK continues to: | ||
|
|
||
| - Call `initialize()`, `shutdown()`, and `on context change` lifecycle methods on the provider | ||
| - Forward provider-emitted events to registered domain and API-level event handlers | ||
| - Run late-attached handlers immediately if the provider is already in the associated state | ||
|
Contributor
There was a problem hiding this comment. major: I believe SDKs can't do this safely as this requires that subscription and emission of the current status is atomic (there should be no new events emitted between reading current status and running the handler) — and you can't guarantee that when another thread is emitting events (unless SDK implements event buffering which is cumbersome).
Member
Author
There was a problem hiding this comment. I agree this is a real concern. With provider-owned state, I don't see how the SDK can safely guarantee atomic late-attached handler execution or short-circuit evaluation without adding significant complexity (e.g. shared locks between the SDK and provider, or subscription-awareness in the provider). That said, this is existing behavior that's important for usability; users need to be able to attach Your suggestion about an event queue is interesting; if the SDK serializes event dispatch through a queue, late-attached handler checks could go through the same queue, which might also address concerns about event handler interleaving. Maybe we need this even though it will be heavy?
Member
Author
There was a problem hiding this comment. I've taken a different approach: #380 (comment)
Contributor
There was a problem hiding this comment. Swift SDK is actually doing this events/subscription serialization with a lock in
Contributor
There was a problem hiding this comment. Thinking about this more, I believe we're confusing "event observing" and "state observing" and this adds a lot of accidental complexity. Many languages have separate tools for a regular channel and a channel that remembers the last value:
The issue is that we want a weird mix of the two: we want it to remember the last state... except "not ready" status because it doesn't have a corresponding event to re-emit)... and don't remember "configuration changed" event (because it doesn't change status)... and "context changed" should be remapped to "ready" (but only if it's a replay and not an online observation). This leads to a couple of issues. The obvious one is that the behavior is convoluted and is complicated to implement. In MultiProvider, we get a mismatch between emitted events and actual provider state (the spec requires MultiProvider to re-emit all events from the underlying providers, but underlying provider going into fatal state does not mean that the whole MultiProvider does). And the last one is that waiting for I think a much cleaner API is to add "status observers" that remember the last status and are guaranteed to fire when the provider switches to the corresponding status (i.e. a successful reconciliation should invoke ready handler). And keep event observers without this last-value memory We can even keep this backward-compatible for SDKs that have subscription topics (JS, Go) by saying that An alternative take is that this issue only exists for languages that have these native tools for broadcast and last-state observers (swift, kotlin), so SDKs decided to expose native observers instead of named event handlers as the spec describes. So now these SDKs are fighting with synchronizing these subjects/flows. As they are already deviating from the spec, perhaps they can deviate further to implement status subscription in addition to events (or alternatively align with the spec and implement named event handlers instead of subject/flow — though this is less idiomatic and will be more cumbersome for users) cc @nicklasl @fabriziodemaria @typotter for your thoughts as this is a pain point in kotlin and swift
Member
Author
There was a problem hiding this comment. I haven't had time to fully consider this, but I think it's close to what I've proposed in my other pr: #385 It doesn't mention the "BehaviorSubject" construct exactly, but describes something basically similar. I agree that we could use those abstractions though. |
||
| - Enforce short-circuit behavior for `NOT_READY` and `FATAL` statuses during flag evaluation | ||
|
Contributor
There was a problem hiding this comment. major: multi-threaded SDKs can't enforce this because of TOCTOU |
||
|
|
||
| #### Deprecation | ||
|
|
||
| The legacy path (SDK-managed status) should be deprecated in the release that introduces the state-managing interface, with removal targeted for the next major version. | ||
| SDK authors should update any first-party providers and provider base classes to implement the new interface. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor/aside: I'd argue that the better design is implementing an adapter that takes a non-
StateManagingProviderand implements theStateManagingProviderinterface. This way, all special-casing/mapping is concentrated in one place, and it is about the only thing that needs to be deleted in the next major release.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC this is essentially what the PoCs already do; FeatureProviderStateManager in Java and providerReference in Go are effectively that adapter/wrapper. They wrap legacy providers and handle state management on their behalf, so downstream SDK code doesn't need to special-case. At the next major version, the wrapper is the only thing that gets deleted.
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to using adapter pattern to adapt legacy interface to the new one. Strictly speaking, none of the PoCs do that.
Kotlin PR is an example.:
LegacyFeatureProviderAdapterwraps a non-state-managing provider and implement state-managing interface (manage status, emits events, etc.). It nicely contains/abstracts the knowledge of non-managing providers, and the rest of openfeature sdk handles all providers equally as if they can manage their state.This also reduces the cyclomatic complexity overall: the adapter knows the wrapped provider doesn't manage status, so it doesn't need any ifs. The SDK can treat all providers as state-managing, so it doesn't need any ifs either. The only
ifis insetProviderto determine whether wrapping is needed.In other PoCs, the provider wrapper now has a double-duty of whatever-it-was-doing-before + adapting legacy providers, so it's riddled with
ifs. And it still doesn't contain the adapting completely and this part doesn't hold:In JS,
OpenFeatureCommonAPIreaches inside withwrappedProvider.delegateManagesStateto determine whether to emit events or not. In Java,ProviderRepositoryreaches in withnewManager.delegateManagesState. Go is doing the checks in both event executor and openfeature API. So we'd need to modify these components as well.It's not a huge deal though, especially for a temporary migration. I find the proper adapter slightly cleaner but it is more work than throwing a bunch of if's in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK, you were just talking about encapsulating this logic more elegantly then? If that's the case I agree. The PoCs were just there to prove the flow was possible, not necessarily in the cleanest way.