fix: provider state ownership via events#385
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the OpenFeature specification to shift the responsibility of emitting lifecycle events from the SDK to the providers. By requiring providers to emit events for status transitions such as initialization and context reconciliation, the SDK can derive provider status directly from the event stream, which helps prevent race conditions in multi-threaded environments. The changes include updated requirements, a new migration guide in Appendix E, and documentation adjustments. Reviewers identified an inconsistency where 'shutdown' was included in lists of methods requiring event emission despite the lack of a defined shutdown event; they provided suggestions to remove 'shutdown' from these requirements and ensure consistent terminology for context changes.
| { | ||
| "id": "Requirement 2.8.1", | ||
| "machine_id": "requirement_2_8_1", | ||
| "content": "The provider MUST emit an event to signal each status transition, including transitions resulting from lifecycle methods (`initialize`, `shutdown`, `on context change`) and spontaneous transitions.", |
There was a problem hiding this comment.
The inclusion of shutdown in the list of lifecycle methods that MUST emit an event is inconsistent with the rest of the specification. There is no defined event for shutdown (e.g., PROVIDER_NOT_READY), and Requirement 1.7.9 states that the SDK updates the status to NOT_READY when the shutdown function terminates. Additionally, the PR description notes that shutdown remains unchanged and the SDK infers the state. Also, the method name should be on context changed to match Requirement 2.6.1.
| "content": "The provider MUST emit an event to signal each status transition, including transitions resulting from lifecycle methods (`initialize`, `shutdown`, `on context change`) and spontaneous transitions.", | |
| "content": "The provider MUST emit an event to signal each status transition, including transitions resulting from lifecycle methods (initialize, on context changed) and spontaneous transitions.", |
|
|
||
| #### Requirement 2.8.1 | ||
|
|
||
| > The provider **MUST** emit an event to signal each status transition, including transitions resulting from lifecycle methods (`initialize`, `shutdown`, `on context change`) and spontaneous transitions. |
There was a problem hiding this comment.
The inclusion of shutdown here is inconsistent with the fact that no shutdown event is defined in the specification. Requirement 1.7.9 confirms that the SDK manages the transition to NOT_READY upon termination of the shutdown function. Also, the method name should be on context changed for consistency with Requirement 2.6.1.
| > The provider **MUST** emit an event to signal each status transition, including transitions resulting from lifecycle methods (`initialize`, `shutdown`, `on context change`) and spontaneous transitions. | |
| The provider MUST emit an event to signal each status transition, including transitions resulting from lifecycle methods (initialize, on context changed) and spontaneous transitions. |
|
|
||
| Providers cannot emit `PROVIDER_CONTEXT_CHANGED` or `PROVIDER_RECONCILING` events. | ||
| These are emitted only by the SDK during context reconciliation. | ||
| Providers must emit events to signal all state transitions, including those resulting from lifecycle methods (initialize, shutdown, reconciliation). |
There was a problem hiding this comment.
As noted in other sections, shutdown does not have a corresponding event and its state transition is inferred by the SDK when the method terminates (per Requirement 1.7.9). It should be removed from this list of examples.
| Providers must emit events to signal all state transitions, including those resulting from lifecycle methods (initialize, shutdown, reconciliation). | |
| Providers must emit events to signal all state transitions, including those resulting from lifecycle methods (initialize, reconciliation). |
Providers emit events to signal all status transitions; the SDK derives provider status from these events rather than inferring from lifecycle method return values. - Add section 2.8 (Provider status) with requirements 2.8.1-2.8.4 - Add Condition 2.8.5 for simple providers (no lifecycle, no events) - Update 1.7.3/4/5 to reference provider events instead of init outcomes - Change 5.1.1 from MAY to MUST; reframe 5.3.1/2 as event-driven - Update 5.3.5 table (PROVIDER_CONTEXT_CHANGED -> READY, PROVIDER_RECONCILING -> RECONCILING) - Add non-normative threading note to 5.3.3 - Add Appendix E (Migrations) for legacy provider compatibility - Fix lint check for non-requirement entries Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
3177a63 to
4734a58
Compare
Providers emit events to signal all status transitions; the SDK derives
provider status from these events rather than inferring from lifecycle
method return values.
Resolves #365
This is an alternative to #380 which took a different approach (adding getState() to providers). That one works but is ~10x larger and introduces concurrent-safety requirements on the provider that don't exist if state lives only in the SDK. Same problem, simpler solution IMO. Happy to discuss tradeoffs.