diff --git a/lib/open_feature/sdk/configuration.rb b/lib/open_feature/sdk/configuration.rb index a48be5b6..67e8010a 100644 --- a/lib/open_feature/sdk/configuration.rb +++ b/lib/open_feature/sdk/configuration.rb @@ -34,6 +34,7 @@ def initialize @provider_state_registry = ProviderStateRegistry.new @client_handlers = {} @client_handlers_mutex = Mutex.new + @dispatch_key = :"openfeature_dispatch_#{object_id}" end def add_hooks(*new_hooks) @@ -60,6 +61,8 @@ def remove_handler(event_type, handler) # @api private def add_client_handler(client, event_type, handler) + @event_dispatcher.validate_handler!(event_type, handler) + @client_handlers_mutex.synchronize do @client_handlers[client] ||= Hash.new { |h, k| h[k] = [] } @client_handlers[client][event_type] << handler @@ -202,7 +205,7 @@ def set_provider_internal(provider, domain:, wait_for_init:) attempt.done.pop raise attempt.error || incomplete_init_error(provider, "initialization did not complete") unless attempt.state == :succeeded - dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY) + dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY, token: attempt) end ensure # Fail an attempt no init took over (e.g. a Timeout hit first), or it would stay running forever @@ -215,7 +218,7 @@ def init_provider(provider, context, domain:, attempt:, raise_on_error: false) # Settle the attempt before handlers run, so a handler can register this provider again. # An abandoned attempt (provider replaced or shut down meanwhile) reports no READY. - dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY) if finish_init_attempt(attempt, :succeeded) + dispatch_provider_event(provider, ProviderEvent::PROVIDER_READY, token: attempt) if finish_init_attempt(attempt, :succeeded) rescue => e # Spec 1.7.5: PROVIDER_FATAL from init makes the provider FATAL error_code = EvaluationError.code_for(e) @@ -228,8 +231,7 @@ def init_provider(provider, context, domain:, attempt:, raise_on_error: false) finish_init_attempt(attempt, :failed, error) dispatch_provider_event(provider, ProviderEvent::PROVIDER_ERROR, - error_code: error_code, - message: e.message) + {error_code: error_code, message: e.message}, token: attempt) raise error if raise_on_error ensure @@ -260,25 +262,56 @@ def finish_init_attempt(attempt, state, error = nil) true end - def dispatch_provider_event(provider, event_type, details = {}) - # Under the replacement lock, so a late event can't re-track a replaced or shut-down provider - @provider_mutex.synchronize do - return unless provider_bound?(provider) + # An event for a provider already dispatching on this call stack is queued behind the current one + def dispatch_provider_event(provider, event_type, details = {}, token: nil) + # The SDK's lifecycle events (with a token) record status when sent, so set_provider_and_wait returns + # with it even from the provider's own handler (1.1.2.4); provider events record it when delivered (5.3.5) + return if token && !accept_provider_event(provider, event_type, details, token) - @provider_state_registry.update_state_from_event(provider, event_type, details) + queues = (Thread.current[@dispatch_key] ||= {}.compare_by_identity) + if (queue = queues[provider]) + queue << [event_type, details.dup, token] + return end - provider_name = extract_provider_name(provider) + queue = queues[provider] = [[event_type, details, token]] + begin + deliver_provider_event(provider, *queue.shift) + until queue.empty? + begin + deliver_provider_event(provider, *queue.shift) + rescue => e + # Its own emitter already returned, so log rather than fail the event that was dispatching + @logger&.error("Queued provider event failed: #{e.message}") + end + end + ensure + queues.delete(provider) + Thread.current[@dispatch_key] = nil if queues.empty? + end + end - event_details = { - provider_name: provider_name - }.merge(details) + def deliver_provider_event(provider, event_type, details, token) + return unless accept_provider_event(provider, event_type, details, token, record_status: token.nil?) + event_details = {provider_name: extract_provider_name(provider)}.merge(details) run_handlers_for_provider(provider, event_type, event_details) end private + # Under the replacement lock, so a late event can't re-track a replaced or shut-down provider + def accept_provider_event(provider, event_type, details, token, record_status: true) + @provider_mutex.synchronize do + return false unless provider_bound?(provider) + # An SDK READY/ERROR from an earlier registration of this provider is stale + return false unless token.nil? || @init_attempts[provider].equal?(token) + + @provider_state_registry.update_state_from_event(provider, event_type, details) if record_status + true + end + end + def provider_bound?(provider) @providers.each_value.any? { |p| p.equal?(provider) } end @@ -325,22 +358,20 @@ def run_handlers_for_provider(provider, event_type, event_details) # Run global handlers (API-level, no domain filtering) @event_dispatcher.trigger_event(event_type, event_details) - # Run client handlers (domain-scoped) - @client_handlers_mutex.synchronize do - @client_handlers.each do |client, handlers_by_event| + # Run client handlers (domain-scoped) outside the lock, so they can add or remove handlers + handlers = @client_handlers_mutex.synchronize do + @client_handlers.flat_map do |client, handlers_by_event| # Check if this client should receive events from this provider client_provider = provider(domain: client.metadata.domain) - next unless client_provider&.equal?(provider) - - # Trigger handlers for this client - handlers = handlers_by_event[event_type] - handlers.each do |handler| - handler.call(event_details) - rescue => e - @logger&.error("Client event handler failed: #{e.message}") - end + client_provider&.equal?(provider) ? handlers_by_event[event_type] : [] end end + + handlers.each do |handler| + handler.call(event_details) + rescue => e + @logger&.error("Client event handler failed: #{e.message}") + end end def run_immediate_handler(event_type, handler, client) @@ -352,8 +383,8 @@ def run_immediate_handler(event_type, handler, client) } if client.nil? - # API-level handler: check all providers - @providers.each do |domain, provider| + # API-level handler: check each bound provider once, however many domains it's bound to + @providers.values.uniq(&:object_id).each do |provider| next unless provider provider_state = @provider_state_registry.get_state(provider) diff --git a/lib/open_feature/sdk/event_dispatcher.rb b/lib/open_feature/sdk/event_dispatcher.rb index e3b70dae..4b59478d 100644 --- a/lib/open_feature/sdk/event_dispatcher.rb +++ b/lib/open_feature/sdk/event_dispatcher.rb @@ -16,14 +16,18 @@ def initialize(logger = nil) end def add_handler(event_type, handler) - raise ArgumentError, "Invalid event type: #{event_type}" unless valid_event?(event_type) - raise ArgumentError, "Handler must respond to call" unless handler.respond_to?(:call) + validate_handler!(event_type, handler) @mutex.synchronize do @handlers[event_type] << handler end end + def validate_handler!(event_type, handler) + raise ArgumentError, "Invalid event type: #{event_type}" unless valid_event?(event_type) + raise ArgumentError, "Handler must respond to call" unless handler.respond_to?(:call) + end + def remove_handler(event_type, handler) return unless valid_event?(event_type) diff --git a/sig/open_feature/sdk/configuration.rbs b/sig/open_feature/sdk/configuration.rbs index 19dceb4d..7b15c2b0 100644 --- a/sig/open_feature/sdk/configuration.rbs +++ b/sig/open_feature/sdk/configuration.rbs @@ -36,7 +36,9 @@ module OpenFeature def finish_init_attempt: (untyped attempt, Symbol state, ?ProviderInitializationError? error) -> bool def abandon_init_attempt: (untyped attempt) -> void def incomplete_init_error: (untyped provider, String reason) -> ProviderInitializationError - def dispatch_provider_event: (untyped provider, String event_type, ?Hash[Symbol, untyped] details) -> void + def dispatch_provider_event: (untyped provider, String event_type, ?Hash[Symbol, untyped] details, ?token: untyped) -> void + def deliver_provider_event: (untyped provider, String event_type, Hash[Symbol, untyped]? details, untyped token) -> void + def accept_provider_event: (untyped provider, String event_type, Hash[Symbol, untyped]? details, untyped token, ?record_status: bool) -> bool def provider_bound?: (untyped provider) -> bool def domain_scoped?: (untyped provider) -> bool def validate_domain_scoped_binding!: (untyped provider, String? domain, bool scoped) -> void diff --git a/sig/open_feature/sdk/event_dispatcher.rbs b/sig/open_feature/sdk/event_dispatcher.rbs index 87f52b80..d6458f90 100644 --- a/sig/open_feature/sdk/event_dispatcher.rbs +++ b/sig/open_feature/sdk/event_dispatcher.rbs @@ -6,6 +6,7 @@ module OpenFeature def initialize: (?untyped? logger) -> void def add_handler: (String event_type, _ToProc handler) -> void + def validate_handler!: (String event_type, untyped handler) -> void def remove_handler: (String event_type, _ToProc handler) -> void def remove_all_handlers: (String event_type) -> void def trigger_event: (String event_type, ?Hash[Symbol, untyped] event_details) -> void diff --git a/spec/open_feature/sdk/configuration_spec.rb b/spec/open_feature/sdk/configuration_spec.rb index 97a89fb2..35ed2f43 100644 --- a/spec/open_feature/sdk/configuration_spec.rb +++ b/spec/open_feature/sdk/configuration_spec.rb @@ -408,6 +408,14 @@ def init(evaluation_context = nil, **opts) = @received = [evaluation_context, op end end + describe "nested event dispatch" do + it "leaves no per-thread state behind once dispatch finishes" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + + expect { configuration.set_provider_and_wait(provider) }.not_to change { Thread.current.keys.size } + end + end + describe "event handler error logging" do it "logs error when event handler fails and logger is present" do provider = OpenFeature::SDK::Provider::NoOpProvider.new diff --git a/spec/specification/events_spec.rb b/spec/specification/events_spec.rb index a065671c..b63196d0 100644 --- a/spec/specification/events_spec.rb +++ b/spec/specification/events_spec.rb @@ -78,6 +78,134 @@ def shutdown # Cleanup OpenFeature::SDK.remove_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, handler) end + + specify "5.1.2: a handler that raises doesn't stop queued events" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + received = [] + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, lambda do |_details| + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) + raise "handler failed" + end) + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_details) { received << :ready }) + received.clear # Ignore the immediate callback during registration + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(received).to eq([:ready]) + end + + specify "5.1.2: a failing queued event doesn't turn a successful init into a failure" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + logger = spy("Logger") + OpenFeature::SDK.configuration.logger = logger + # Non-Hash details make the queued event fail while its handlers are prepared + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_details) { provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, nil) }) + + expect { OpenFeature::SDK.set_provider_and_wait(provider) }.not_to raise_error + expect(OpenFeature::SDK.configuration.provider_state(provider)).to eq(OpenFeature::SDK::ProviderState::STALE) + expect(logger).to have_received(:error).with(/Queued provider event failed/) + ensure + OpenFeature::SDK.configuration.logger = nil + end + + specify "5.1.2: a queued event is delivered with the details it was emitted with" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + received = nil + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, lambda do |_details| + details = {message: "first"} + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_CONFIGURATION_CHANGED, details) + details[:message] = "changed" + end) + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_CONFIGURATION_CHANGED, ->(details) { received = details[:message] }) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(received).to eq("first") + end + + specify "5.1.2: a queued event is dropped when its provider was replaced meanwhile" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + names = [] + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, lambda do |_details| + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) + OpenFeature::SDK.set_provider_and_wait(OpenFeature::SDK::Provider::NoOpProvider.new) + end) + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(details) { names << details[:provider_name] }) + names.clear # Ignore the immediate callback during registration + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(names).to eq(["No-op Provider"]) + end + + specify "5.1.2: set_provider_and_wait for another provider from inside a handler returns READY" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + other = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + states = [] + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, lambda do |_details| + OpenFeature::SDK.set_provider_and_wait(other, domain: "x") + states << OpenFeature::SDK.configuration.provider_state(other) + end) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(states).to eq([OpenFeature::SDK::ProviderState::READY]) + end + + specify "5.1.2: a client handler that raises is logged and doesn't stop the other client handlers" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + logger = spy("Logger") + OpenFeature::SDK.configuration.logger = logger + received = [] + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, ->(_details) { raise "handler failed" }) + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, ->(_details) { received << :stale }) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(received).to eq([:stale]) + expect(logger).to have_received(:error).with(/Client event handler failed: handler failed/) + ensure + OpenFeature::SDK.configuration.logger = nil + end + + specify "5.1.2: a client handler can add another handler while handling an event" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + received = [] + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, lambda do |_details| + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(_ready_details) { received << :ready }) + end) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) + + expect(received).to eq([:ready]) + end + + specify "5.1.2: a client handler can emit an event for another provider" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + other = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider, domain: "a") + OpenFeature::SDK.set_provider_and_wait(other, domain: "b") + client = OpenFeature::SDK.build_client(domain: "a") + other_client = OpenFeature::SDK.build_client(domain: "b") + received = [] + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, ->(_details) { other.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) }) + other_client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, ->(_details) { received << :stale }) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + + expect(received).to eq([:stale]) + end end context "Requirement 5.1.3" do @@ -223,6 +351,13 @@ def init(_evaluation_context) client_handlers = OpenFeature::SDK.configuration.instance_variable_get(:@client_handlers) expect(client_handlers[client][OpenFeature::SDK::ProviderEvent::PROVIDER_READY]).to be_empty end + + specify "rejects a client handler for an unknown event type or one that can't be called" do + client = OpenFeature::SDK.build_client + + expect { client.add_handler("NOPE", ->(_details) {}) }.to raise_error(ArgumentError, /Invalid event type/) + expect { client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) }.to raise_error(ArgumentError, /Handler must respond to call/) + end end context "Requirement 5.2.2" do @@ -330,6 +465,29 @@ def init(_evaluation_context) end end + context "Requirement 5.2.7" do + after { OpenFeature::SDK.configuration.logger = nil } + + specify "5.2.7: a client handler can remove itself while handling an event" do + logger = spy("Logger") + OpenFeature::SDK.configuration.logger = logger + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + calls = 0 + handler = lambda do |_details| + calls += 1 + client.remove_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, handler) + end + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE, handler) + + 2.times { provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) } + + expect(calls).to eq(1) + expect(logger).not_to have_received(:error) + end + end + context "Requirement 5.3.1" do specify "If the provider's initialize function terminates normally, PROVIDER_READY handlers MUST run" do ready_event_received = false @@ -370,6 +528,17 @@ def init(_evaluation_context) OpenFeature::SDK.remove_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, handler) end + + specify "5.3.3: an API handler runs once for a READY provider bound to two domains" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider, domain: "a") + OpenFeature::SDK.set_provider_and_wait(provider, domain: "b") + names = [] + + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_READY, ->(details) { names << details[:provider_name] }) + + expect(names.count("In-memory Provider")).to eq(1) + end end context "Client-level handlers" do @@ -390,4 +559,82 @@ def init(_evaluation_context) end end end + + context "Requirement 5.3.5" do + specify "1.1.2.4: set_provider_and_wait from the provider's own ERROR handler returns with the provider READY" do + outcomes = Queue.new + outcomes << :fail << :ok + provider = Class.new(OpenFeature::SDK::Provider::InMemoryProvider) do + define_method(:init) { |_evaluation_context = nil| raise "init failed" if outcomes.pop == :fail } + end.new + retried = false + state_after_retry = nil + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR, lambda do |_details| + next if retried + + retried = true + OpenFeature::SDK.set_provider_and_wait(provider) + state_after_retry = OpenFeature::SDK.configuration.provider_state(provider) + end) + + expect { OpenFeature::SDK.set_provider_and_wait(provider) }.to raise_error(OpenFeature::SDK::ProviderInitializationError) + expect(state_after_retry).to eq(OpenFeature::SDK::ProviderState::READY) + end + + [[:ERROR, :READY], [:READY, :STALE]].each do |first, following| + specify "5.3.5: a nested event for the same provider is delivered after the current one (#{first}, then #{following})" do + first_event, following_event = [first, following].map { |name| OpenFeature::SDK::ProviderEvent.const_get(:"PROVIDER_#{name}") } + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_STALE) + client = OpenFeature::SDK.build_client + events = [] + OpenFeature::SDK.add_handler(first_event, ->(_details) { provider.emit_event(following_event) }) + client.add_handler(first_event, ->(_details) { events << first }) + client.add_handler(following_event, ->(_details) { events << following }) + events.clear # Ignore immediate callbacks during registration + + provider.emit_event(first_event) + + expect(events).to eq([first, following]) + end + end + + specify "5.3.5: client handlers of the outer event observe the outer event's status" do + provider = OpenFeature::SDK::Provider::InMemoryProvider.new + OpenFeature::SDK.set_provider_and_wait(provider) + client = OpenFeature::SDK.build_client + statuses = [] + OpenFeature::SDK.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR, ->(_details) { provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_READY) }) + client.add_handler(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR, ->(_details) { statuses << client.provider_status }) + + provider.emit_event(OpenFeature::SDK::ProviderEvent::PROVIDER_ERROR) + + expect(statuses).to eq([OpenFeature::SDK::ProviderState::ERROR]) + end + + specify "5.3.5: a stale READY from an earlier registration is dropped" do + gates = [Queue.new, Queue.new] + first_init, second_init = gates + provider = Class.new(OpenFeature::SDK::Provider::InMemoryProvider) do + define_method(:init) { |_evaluation_context = nil| gates.shift.pop } + end.new + init_threads = [] + allow(Thread).to receive(:new).and_wrap_original { |original, *args, &block| original.call(*args, &block).tap { |t| init_threads << t } } + + OpenFeature::SDK.set_provider(provider) + 500.times { first_init.num_waiting.positive? ? break : sleep(0.01) } + expect(first_init.num_waiting).to eq(1) + OpenFeature::SDK.set_provider(OpenFeature::SDK::Provider::NoOpProvider.new) + OpenFeature::SDK.set_provider(provider) + + first_init << :ok + expect(init_threads.first.join(5)).to be(init_threads.first) + expect(OpenFeature::SDK.configuration.provider_state(provider)).to eq(OpenFeature::SDK::ProviderState::NOT_READY) + + second_init << :ok + expect(init_threads.last.join(5)).to be(init_threads.last) + expect(OpenFeature::SDK.configuration.provider_state(provider)).to eq(OpenFeature::SDK::ProviderState::READY) + end + end end