feat!: add provider eventing, remove setProvider timeout #207
Conversation
Summary of ChangesHello @sameerank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant and well-executed enhancement that introduces a provider eventing system, aligning the Ruby SDK more closely with the OpenFeature specification. The core improvement is fixing set_provider_and_wait to correctly block until a provider is ready, which is achieved through a robust, thread-safe event mechanism using EventEmitter, ProviderStateRegistry, and new provider interfaces (EventHandler, StateHandler). The changes are thoughtfully designed to be backward-compatible, and the comprehensive new test suite, including specification compliance tests, is excellent.
My feedback focuses on a few areas to further improve robustness and maintainability: refining exception handling to be more specific, enhancing debuggability by preserving original error information in event payloads, and cleaning up a small amount of unused code.
There was a problem hiding this comment.
Code Review
This pull request is a fantastic and significant improvement. It correctly implements the provider eventing system as specified by OpenFeature, fixing the broken behavior of set_provider_and_wait. The introduction of EventEmitter, EventHandler, and state management is well-structured and thread-safe. The changes are also backward compatible, which is a huge plus. The test coverage for the new asynchronous logic is thorough. I've found a few minor but important issues related to overly broad exception handling (rescue => e) and a small redundancy in event payload creation, which I've commented on. Once these are addressed, this PR will be in excellent shape.
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a provider eventing system, aligning the Ruby SDK more closely with the OpenFeature specification. The implementation correctly fixes the behavior of set_provider_and_wait and makes set_provider non-blocking, which is a great improvement for providers requiring asynchronous initialization. The code is well-structured, with clear separation of concerns into new modules and classes for eventing and state management. The test coverage is comprehensive and thoughtful, especially the tests for backward compatibility and specification compliance.
I have identified a few areas for improvement, primarily focusing on enhancing thread safety and encapsulation. Specifically, I've pointed out a potential race condition in the error handling of set_provider_and_wait and suggested a way to improve how the logger is managed. These changes will make the implementation more robust in concurrent environments. Overall, this is a very strong contribution.
- Add ProviderEvent module with event type constants (PROVIDER_READY, etc.) - Add ProviderState module with state constants (NOT_READY, READY, ERROR, etc.) - Implement EventEmitter class for thread-safe pub-sub event handling - Add EventToStateMapper for mapping events to provider states - Include comprehensive test coverage for all components This introduces the event infrastructure needed for OpenFeature spec-compliant provider lifecycle management. The implementation is additive only, maintaining full backward compatibility with existing provider behavior. Signed-off-by: Sameeran Kunche <[email protected]>
Add mixins and classes to support provider lifecycle management: - StateHandler: Basic provider lifecycle interface with init/shutdown - EventHandler: Event emission capability for providers - ContextAwareStateHandler: Timeout support for initialization - ProviderStateRegistry: Thread-safe provider state tracking - EventAwareNoOpProvider: Example implementation demonstrating interfaces These interfaces enable providers to emit lifecycle events and support both synchronous and asynchronous initialization patterns while maintaining backward compatibility with existing providers. Signed-off-by: Sameeran Kunche <[email protected]>
- Fix context_aware_state_handler_spec to expect both positional and keyword arguments - Add newlines to end of provider files for consistency - Add arm64-darwin-24 platform support to Gemfile.lock Signed-off-by: Sameeran Kunche <[email protected]>
…ycle Refactor provider lifecycle to be truly asynchronous: - set_provider now returns immediately and initializes providers in background - set_provider_and_wait uses event system to block until initialization completes - Providers emit PROVIDER_READY/PROVIDER_ERROR events on initialization - Event handlers enable non-blocking application startup patterns - Maintains backward compatibility with existing providers The implementation follows OpenFeature specification requirements: - setProvider is non-blocking (returns immediately) - setProviderAndWait blocks until provider is ready or errors - Provider state transitions are tracked via events - Failed initialization properly restores previous provider state Added comprehensive test coverage for async behavior, error handling, event emission, and backward compatibility scenarios. Signed-off-by: Sameeran Kunche <[email protected]>
…fecycle Add specification tests for requirements implemented in this branch: - Requirement 1.1.2.4: set_provider_and_wait blocking behavior - Requirement 2.4.2.1: Provider initialization error handling - Requirement 5.x: Provider lifecycle events and handlers The tests follow Ruby SDK's existing pattern of using RSpec contexts to organize tests by specification requirements. This provides clear traceability between implementation and specification compliance. Also exposed add_handler/remove_handler methods on the SDK API to support event specification tests. Signed-off-by: Sameeran Kunche <[email protected]>
- Remove verbose method documentation from event_emitter.rb - Remove detailed parameter descriptions from provider modules - Remove implementation comments from configuration.rb - Keep only essential comments explaining non-obvious behavior - Maintain brief class/module descriptions where helpful Follows minimalist Ruby documentation style used throughout the codebase Signed-off-by: Sameeran Kunche <[email protected]>
- Replace bare rescue with rescue StandardError in configuration.rb - Add ProviderInitializationFailure class for structured error details - Pass error codes and messages in events (not full exceptions) - Add configurable logger support to Configuration and EventEmitter - Remove unused state_from_error and fatal_error? methods These changes address all issues identified by the Gemini code review bot while maintaining alignment with other OpenFeature SDKs (Go and Java). Signed-off-by: Sameeran Kunche <[email protected]>
…l value - ContextAwareStateHandler was just a placeholder that only included StateHandler - Other SDKs (Go, Java) don't expose timeout methods on provider interfaces - Timeouts are controlled at the API level (set_provider_and_wait), not provider level - Providers can implement their own internal timeout logic if needed - Also removed unused state_from_error tests that were causing failures Signed-off-by: Sameeran Kunche <[email protected]>
- Replace overly broad 'rescue => e' with 'rescue StandardError => e' in 3 files to avoid catching system-level exceptions like Interrupt and SystemExit - Remove redundant provider merge in event_handler.rb since provider is already passed as first argument to dispatch_event - Update tests to match the simplified event dispatching Signed-off-by: Sameeran Kunche <[email protected]>
- Fix race conditions in set_provider_and_wait by adding revert_provider_if_current method that only reverts if the current provider matches the one being set - Improve encapsulation by adding attr_writer :logger to EventEmitter instead of using instance_variable_set - Add logging for provider shutdown errors for better debugging visibility - Update test coverage in PR description to 98.73% (468/474 LOC) Signed-off-by: Sameeran Kunche <[email protected]>
sameerank
force-pushed
the
feat/add-provider-eventing
branch
from
d5d4c34 to
6eefe78
Compare
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive pull request that addresses a critical issue with set_provider_and_wait and brings the Ruby SDK much closer to the OpenFeature specification for provider eventing. The introduction of an asynchronous, event-driven provider lifecycle is a significant improvement.
I'm impressed by the thoroughness of the implementation, including:
- Thread-safe event emitter and state management.
- Correct handling of asynchronous initialization and error conditions.
- Thoughtful approach to backward compatibility, allowing old providers to function without modification.
- Extensive test coverage, including tests for concurrency, which gives high confidence in these complex changes.
I have one suggestion regarding the creation of the event dispatcher in Configuration#set_provider to improve code clarity and maintainability.
Overall, this is a high-quality contribution that significantly enhances the SDK's functionality and robustness.
- Replace dynamic Object creation with singleton methods with a proper private inner class - Improves code clarity, maintainability, and performance - Follows Ruby idioms for encapsulation - Suggested by Gemini code review Signed-off-by: Sameeran Kunche <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive provider eventing system, which is a significant and well-executed feature. It successfully fixes the behavior of set_provider_and_wait to be truly blocking and aligns the SDK with the OpenFeature specification for provider events. The implementation includes a thread-safe event emitter, new provider mixins for lifecycle management, and asynchronous provider initialization, all supported by a thorough test suite. I've identified a couple of critical scoping issues that would cause runtime errors and a minor opportunity for code cleanup. Overall, this is an excellent contribution to the SDK.
…tests - Remove EventAwareNoOpProvider as it doesn't exist in other OpenFeature SDKs - Rename provider_backward_compatibility_spec.rb to provider_compatibility_spec.rb - Reorganize tests into three logical sections: - Providers Without Event Capabilities - Mixed Provider Usage - Provider Interface Detection - Update all references and imports accordingly - Maintain 99.13% test coverage Signed-off-by: Sameeran Kunche <[email protected]>
There was a problem hiding this comment.
Code Review
This is a fantastic pull request that brings the Ruby SDK much closer to the OpenFeature specification for provider events and lifecycle. The implementation of an async set_provider and a truly blocking set_provider_and_wait using a thread-safe event system is well-executed. The code is clean, modular, and impressively well-tested, covering backward compatibility, thread safety, and specification requirements.
I've found two potential race conditions with high severity that should be addressed:
- In
Configuration#set_provider_and_wait, there's a race condition that could lead to incorrect provider state restoration on initialization failure. - In
Provider::EventHandler#emit_event, a race condition could lead to aNoMethodErrorif a provider is detached concurrently.
After addressing these concurrency issues, this PR will be a very strong and valuable addition to the SDK. Great work on the thoroughness and quality of this change.
- Fix race condition in set_provider_and_wait where stale provider could be restored - Modified set_provider to return the old provider atomically - set_provider_and_wait now uses the atomically-retrieved old provider - Fix race condition in EventHandler#emit_event - Store @event_dispatcher in local variable before check and use - Prevents NoMethodError if detach is called between check and method call These changes ensure thread-safe operation under concurrent access patterns. Signed-off-by: Sameeran Kunche <[email protected]>
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive implementation of the provider eventing system, aligning the Ruby SDK more closely with the OpenFeature specification. The changes are well-structured, thread-safe, and maintain backward compatibility. The introduction of EventEmitter, StateHandler, and EventHandler provides a solid foundation for asynchronous providers. The new test suites are thorough and cover many edge cases, which is great to see.
I've found one critical issue related to handling nil providers during shutdown and have one suggestion to improve the maintainability of the new test files. Overall, this is a very strong contribution.
Critical fixes: - Fix NoMethodError when old_provider is nil during shutdown - Use safe navigation operators (&.) for nil provider handling - Add test coverage for setting provider to domain with no previous provider API improvements: - Refactor API class to follow OpenFeature SDK patterns - Replace long def_delegators list with explicit methods - Add clear_all_handlers method for improved test cleanup - Align with Go SDK's individual function approach vs bulk delegation All tests passing with 98.74% coverage maintained. Signed-off-by: Sameeran Kunche <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive event handling and provider state management system to the OpenFeature SDK. The set_provider method is now asynchronous, initializing providers in a separate thread, while a new set_provider_and_wait method provides blocking initialization with timeout and error handling, including reverting to the previous provider on failure. This is achieved through new EventEmitter and ProviderStateRegistry classes, along with EventHandler and StateHandler mixins for providers to implement event emission and lifecycle methods. The API now exposes methods for managing event handlers and configuring the logger. Review comments suggest enhancing error and warning messages in Configuration by including the provider's class name for improved debugging, and raise a concern about the fragility of relying on init method arity for argument handling.
Address Gemini code review feedback:
Error message improvements:
- Include provider class names in all error messages for better debugging
- 'Provider #{provider.class.name} initialization failed: #{message}'
- 'Provider #{provider.class.name} initialization timed out after #{timeout} seconds'
Init parameter handling improvements:
- Replace fragile arity check with robust try/fallback approach
- Always attempt init(evaluation_context) first (OpenFeature standard)
- Fallback to init() only for legacy providers with 0-arity methods
- Properly handle edge cases like providers expecting 2+ parameters
- Update InMemoryProvider to accept optional evaluation_context parameter
This ensures backward compatibility while aligning with OpenFeature spec
and providing clear error messages for debugging multi-provider scenarios.
Signed-off-by: Sameeran Kunche <[email protected]>
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed feature addition. It correctly implements the provider eventing system according to the OpenFeature specification, which fixes the behavior of set_provider_and_wait. The use of Thread for async initialization, Queue for synchronization, and Mutex for protecting shared state is well-handled. The new modules and classes (EventEmitter, EventHandler, ProviderStateRegistry, etc.) are well-structured and promote good design patterns. Backward compatibility for existing providers is also thoughtfully implemented. The test coverage is excellent, with new unit tests, async-specific tests, and specification compliance tests. I have a couple of suggestions for minor refactoring to improve robustness and simplify the code, but overall this is a high-quality contribution.
- Replace fragile error message matching with robust method parameter inspection - Simplify EventToStateMapper by removing EventDetails class and working with hashes directly - Standardize on symbol keys for event details since they are generated internally - Remove unnecessary string key support to reduce complexity Signed-off-by: Sameeran Kunche <[email protected]>
sameerank
force-pushed
the
feat/add-provider-eventing
branch
from
65ee77b to
2611c40
Compare
There was a problem hiding this comment.
Code Review
This is an excellent pull request that implements a comprehensive provider eventing system, correctly fixing the behavior of set_provider_and_wait. The changes are well-structured, thoroughly tested, and align with the OpenFeature specification. The use of background threads for initialization, Queue for synchronization, and careful handling of backward compatibility are all well-executed. My feedback is minor, focusing on improving consistency by using defined constants instead of string literals in a couple of places.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and well-designed provider eventing system, aligning the Ruby SDK more closely with the OpenFeature specification. The changes correctly implement asynchronous provider initialization, fix the behavior of set_provider_and_wait to be truly blocking, and add robust state and event management. The implementation demonstrates strong attention to thread safety, backward compatibility, and thorough testing. The code is of high quality, and the new features are a significant improvement to the SDK.
Remove unnecessary inline comments that simply restate obvious constant names and method purposes. Add proper OpenFeature specification URL reference for provider states. Changes: - Remove redundant comments from ProviderEvent constants - Remove redundant comments from ProviderState constants - Remove unnecessary section headers in API class - Add OpenFeature spec URL for provider states documentation Code is now cleaner and self-documenting while maintaining valuable specification references. Signed-off-by: Sameeran Kunche <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
===========================================
+ Coverage 99.61% 100.00% +0.38%
===========================================
Files 17 23 +6
Lines 259 446 +187
===========================================
+ Hits 258 446 +188
+ Misses 1 0 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
beeme1mr
left a comment
beeme1mr
left a comment
There was a problem hiding this comment.
Hey @sameerank, thanks for the PR. I quickly reviewed the change and left a comment on the readme. I'm not very experienced with Ruby and the PR is quite large so I'm going to see if one of my colleagues can help review. Thanks again!
There was a problem hiding this comment.
Please update the feature table with the new status and update the eventing section in the readme.
There was a problem hiding this comment.
Updated!
I was initially going to use the PROVIDER_CONTEXT_CHANGED and PROVIDER_RECONCILING are not implemented as outlined in Req 5.3.5. However, I see that eventing is marked as ✅ in the Java or Go SDKs so it seems those are optional
There was a problem hiding this comment.
CONTEXT_CHANGED and RECONCILING only make sense in static-context (client) paradigm. Server SDKs don't need to support these.
Providers built to conform to the static context paradigm feature two additional events: PROVIDER_RECONCILING and PROVIDER_CONTEXT_CHANGED.
And section 5.3.4 is conditioned on static-context paradigm.
- Fix string literal style preferences (single -> double quotes) - Correct hash syntax and spacing formatting - Remove trailing whitespace and useless assignments - Ensure all files comply with Standard Ruby configuration - Resolve 550+ lint violations identified in CI logs All 52 files now pass Standard Ruby linting with 0 offenses detected. This ensures consistency with CI pipeline linting configuration. Signed-off-by: Sameeran Kunche <[email protected]>
- Mark eventing feature as completed (✅) in features table - Add eventing section with API-level event handler examples - Document EventHandler mixin for provider event emission - Show proper handler management with variable references for removal - Clarify original_error behavior for ProviderInitializationError - Align documentation with OpenFeature specification and other SDKs Provides users with clear guidance on implementing event-driven provider lifecycle management in their applications. Signed-off-by: Sameeran Kunche <[email protected]>
- Move clear_all_handlers in API class to private section - Move handler_count and total_handler_count in Configuration to private - Update tests to use send() for accessing private testing methods - Ensure ProviderEventDispatcher remains internal implementation detail These methods are testing utilities not part of the OpenFeature specification and should not be exposed in the public API. Maintains 100% test coverage and functionality while providing cleaner public interface. Signed-off-by: Sameeran Kunche <[email protected]>
- Remove StateHandler module and related tests - Update provider compatibility test to reflect duck typing usage - Update PR description to remove StateHandler references - No functional changes - duck typing already used in SDK StateHandler provided no real value since Ruby's duck typing makes interfaces optional. Providers can implement init/shutdown directly without any mixin. This simplifies the PR while maintaining full functionality and backward compatibility. Signed-off-by: Sameeran Kunche <[email protected]>
|
@sameerank Hey! I'm also not the best with Ruby, but I authored a good deal of the specification, so I will do by best to review this at least from an "OpenFeature semantics" perspective. Expect a review from me tomorrow, and many thanks! |
sameerank
force-pushed
the
feat/add-provider-eventing
branch
from
95d63f0 to
e346c45
Compare
…trol Replace event-based flow control in set_provider_and_wait with direct blocking initialization following Go/Java SDK patterns. - Add wait_for_init parameter to set_provider_internal - Remove timeout and event handler cleanup from set_provider_and_wait - Extract init_provider helper for unified initialization logic - Preserve event emission in both sync and async paths - Remove unused private methods Reduces complexity by 118 lines while maintaining 100% test coverage. Signed-off-by: Sameeran Kunche <[email protected]>
sameerank
force-pushed
the
feat/add-provider-eventing
branch
from
e346c45 to
e5ba150
Compare
dd-oleksii
left a comment
dd-oleksii
left a comment
There was a problem hiding this comment.
I've left a couple of nits and corner cases but looks good overall.
| # Note: original_error is only present for timeout errors, nil for provider event errors | ||
| puts "Original error: #{e.original_error}" if e.original_error |
There was a problem hiding this comment.
minor: I think it makes sense to always set original_error for ergonomics and semantics (it's a bit surprising that it is not set when there is an underlying error). The fact that it's now nilable is arguably a breaking change as well.
| <!-- TODO: code example of a PROVIDER_CONFIGURATION_CHANGED event for the client and a PROVIDER_STALE event for the API --> | ||
| def init(evaluation_context) | ||
| # Start background process to monitor for configuration changes | ||
| # Note: SDK automatically emits PROVIDER_READY when init completes successfully |
There was a problem hiding this comment.
minor: this note seems inaccurate. PROVIDER_READY is only emitted automatically iff the provider does not include the EventHandler mixin. If the mixin is used, the provider must emit its own PROVIDER_READY event. (Which this example doesn't seem to do?)
There was a problem hiding this comment.
Whether or not the provider implements events, the SDK should automatically emit READY/ERROR events when the init function succeeds/fails (raises exception in this case). More here: https://github.com/open-feature/ruby-sdk/pull/207/changes#r2624429319
| case event_type | ||
| when ProviderEvent::PROVIDER_READY, ProviderEvent::PROVIDER_CONFIGURATION_CHANGED | ||
| ProviderState::READY | ||
| when ProviderEvent::PROVIDER_STALE | ||
| ProviderState::STALE | ||
| when ProviderEvent::PROVIDER_ERROR | ||
| state_from_error_event(event_details) | ||
| else | ||
| ProviderState::NOT_READY | ||
| end | ||
| end |
There was a problem hiding this comment.
major(spec): according Events | OpenFeature, PROVIDER_CONFIGURATION_CHANGED must not update the state. The default shall also be "no state changes."
| def set_provider_and_wait(provider, domain: nil) | ||
| @provider_mutex.synchronize do | ||
| old_provider = @providers[domain] | ||
| set_provider_internal(provider, domain: domain, wait_for_init: true) |
There was a problem hiding this comment.
nitpick: given how set_provider_internal is used (always acquiring mutex for a single call), it almost looks like this method should acquire the mutex internally. This would also make it easier to read that method (don't need to look back at all call sites to confirm that modifying @providers is safe).
It might also use a shorter synchronize block (e.g. calling init_provider inside the synchronize block seems like it can be problematic if provider blocks).
| def set_provider_and_wait(provider, domain: nil, timeout: 30) | ||
| def set_provider_and_wait(provider, domain: nil) |
There was a problem hiding this comment.
major: removing the timeout parameter is a breaking change. Is this intended?
There was a problem hiding this comment.
Very good observation! It is a breaking change, but it's a divergence from the spec. I would actually recommend removing it. We instead prefer to have providers handle this themselves, with some kind of timeout, ideally specified in their options/constructors.
Simply adding a ! to the title (feat!: ...) will mark this change as breaking, which is fine since this is a <1.0 SDK.
| include OpenFeature::SDK::Provider::EventHandler | ||
|
|
||
| define_method :init do |_evaluation_context| | ||
| sleep(0.05) # Simulate some initialization time |
There was a problem hiding this comment.
minor: small sleeps in tests add up rather quickly. It's best if we can avoid sleeps and instead use async communication or time mocking.
| def attach(event_dispatcher) | ||
| @event_dispatcher = event_dispatcher | ||
| end | ||
|
|
||
| def detach | ||
| @event_dispatcher = nil | ||
| end |
There was a problem hiding this comment.
minor: attach/detach seem like internal methods (we don't expect users calling them) — shall we make them private or mark them internal?
| | ❌ | [Logging](#logging) | Integrate with popular logging packages. | | ||
| | ✅ | [Domains](#domains) | Logically bind clients with providers. | | ||
| | ❌ | [Eventing](#eventing) | React to state changes in the provider or flag management system. | | ||
| | ✅ | [Eventing](#eventing) | React to state changes in the provider or flag management system. | |
There was a problem hiding this comment.
major(spec): one thing that seems to be missing is client-level event handlers. The user shall be able to attach event handler to specific clients instead of globally.
There was a problem hiding this comment.
Correct: https://openfeature.dev/specification/sections/events#requirement-521
A corollary to this is "domain scoping" - a client bound to domain x should not get events originating from a provider from domain y. Additionally, late binding to domains should be observed. For example, initially a client bound to domain x will use the default provider, but if a new provider is bound to domain x, that becomes the provider from which the client sources its events.
| def add_handler(event_type, handler) | ||
| @event_emitter.add_handler(event_type, handler) | ||
| end |
There was a problem hiding this comment.
major(spec):
Handlers attached after the provider is already in the associated state, MUST run immediately.
There was a problem hiding this comment.
Ya I noticed this as well due to a skipped test case: #207 (comment)
| end | ||
| rescue => e | ||
| dispatch_provider_event(provider, ProviderEvent::PROVIDER_ERROR, | ||
| error_code: Provider::ErrorCode::PROVIDER_FATAL, |
There was a problem hiding this comment.
major: PROVIDER_FATAL seems too harsh as it signal irrecoverable error. The provider might still be trying to initialize and emit PROVIDER_READY later, so non-fatal error is a safer choice.
| if wait_for_init | ||
| init_provider(provider, context_for_init, raise_on_error: true) | ||
| else | ||
| Thread.new do | ||
| init_provider(provider, context_for_init, raise_on_error: false) | ||
| end | ||
| end |
There was a problem hiding this comment.
minor: does it make sense to always initialize provider in a new thread and for set_provider_and_wait to wait for PROVIDER_READY event? Otherwise, set_provider_and_wait does not wait for async-initializing providers (which is its major reasons to exist).
| unless provider.is_a?(Provider::EventHandler) | ||
| dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY) | ||
| end | ||
| rescue => e | ||
| dispatch_provider_event(provider, ProviderEvent::PROVIDER_ERROR, | ||
| error_code: Provider::ErrorCode::PROVIDER_FATAL, | ||
| message: e.message) | ||
|
|
There was a problem hiding this comment.
The emission of the READY/ERROR events should occur regardless of whether or not the provider is capable of emitting its own events.
The SDK should always emit either an init or error depending on the normal/abnormal termination of the provider's init method. If the provider has no init method, the SDK should just emit READY at the same point the method would have been called.
This is outlined here in section 5.3.
A bit more background:
The provider can "spontaneously" emit events (for instance, an error event if a persistent connection is lost) but the SDK emits events that correspond to the provider lifecycle and actions the application integrator has initiated (such as the READY event expected after a provider is set and initialized).
There was a problem hiding this comment.
How should this work with asynchronous provider initialization?
I'm not too familiar with Ruby concurrency model, so sorry if my question does not make sense. My understanding is that set_provider should not block the thread (even if initialization takes a while) but set_provider_and_wait should. In languages that have promises, the SDK would wait for the initialization promise to resolve before emitting event, which may be much later that initialization function return.
Ruby doesn't have promises, so the options I see are:
- wait for provider event before declaring it ready
- let the provider block in initialization method
- the current PR is a mix of the two, depending on whether provider is able to emit events
From what I gathered, the second option is preferred because we can't rule out a method blocking the thread and blocking seems very common in Ruby, so it's likely that many providers already implement it this way. I think it makes sense to ask initialize to always block before it's ready/error and always emit events after initialize returns. Now, if I'm correct that we want set_provider to be non-blocking, this means that we need to initialize provider in a new Thread, which seems suboptimal but it's not too bad.
Does this match your expectations?
| context "Requirement 5.3.3" do | ||
| specify "Handlers attached after the provider is already in the associated state, MUST run immediately" do | ||
| # NOTE: This requirement is about handlers running immediately when attached after a provider | ||
| # is already in the associated state (e.g., READY). This feature is not yet implemented | ||
| # in the Ruby SDK. Handlers are only triggered by state transitions, not by current state. | ||
| skip "Immediate handler execution for current state not yet implemented" | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we need to implement this as well before a release. If this PR is getting too big, I don't mind if it gets done in a separate one, but I don't want it to linger un-implemented too long.
|
@sameerank excellent work, this is great progress. I've left some feedback. The main things missing that I can see are around automatic event emission at provider init, and missing client event handlers (as well as the associated domain scoping, which can be tricky to implement). Please check the Java and JS SDKs for tests and implementation around these, as they may be helpful. Thanks so much for your efforts! ❤️ |
|
@dd-oleksii Thanks for your detailed reviews as well 🙏 |
toddbaert
changed the title
5 participants
This PR
Fixes the behavior of
set_provider_and_waitand implements eventing to trigger event handlers on changes to provider state.What was wrong?
Both
set_providerandset_provider_and_waitinitialize synchronously. By fixingset_providerto be non-blocking, it is also useful to have eventing, so the SDK can be used to trigger handlers according to background provider updates.The Ruby SDK was missing the provider lifecycle event system defined in the OpenFeature specification, which other SDKs use for monitoring provider state changes and enabling event-driven workflows.
What this PR does:
set_providernon-blocking - returns immediately and initializes in a background thread, enabling async operationPROVIDER_READY,PROVIDER_ERROR, etc.) as defined in the Events specification to enable event-driven workflowsNOT_READY,READY,ERROR,FATAL,STALE) as defined in the Provider Status specificationtimeoutparameter fromset_provider_and_waitto align with other OpenFeature SDKs (Go and Java don't support timeouts)Key changes:
Event System Foundation:
EventEmitter- thread-safe pub/sub for provider events (based on Go SDK's eventExecutor)ProviderEventconstants matching the Events specificationEventToStateMapperfor state transitions per Provider Status specificationProvider Interface (opt-in via mixin, based on Go SDK patterns):
EventHandler- event emission capabilities based on Go's EventHandlerProvider Initialization:
init()methods are expected to block until readyOpenFeature::SDK.set_provider(provider)- runs init in background thread (non-blocking API call)OpenFeature::SDK.set_provider_and_wait(provider)- runs init synchronously and blocks until complete (Req 1.1.2.4)Error Handling and Logging:
StandardErroronly, not all exceptions)ProviderInitializationErrorexception forset_provider_and_waitfailures with structured error details (provider,error_code,original_error){error_code:, message:}) similar to Java's ProviderEventDetailsconfig.logger = my_logger) - Note: Go SDK deprecated SetLogger, Java uses SLF4J facadeTesting:
Related Issues
Fixes #51
Notes
timeoutparameter inset_provider_and_waitFollow-up Tasks
How to test