From 4b643802f2549aed6cf97a93644c7f53c4768e10 Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Tue, 12 Sep 2023 14:40:03 -0700 Subject: [PATCH 1/2] fix: ensure event handlers run immediately if able Signed-off-by: Craig Pastro --- pkg/openfeature/event_executor.go | 32 ++-- pkg/openfeature/event_executor_test.go | 202 +++++++++++++++++++++++++ pkg/openfeature/provider.go | 1 + 3 files changed, 225 insertions(+), 10 deletions(-) diff --git a/pkg/openfeature/event_executor.go b/pkg/openfeature/event_executor.go index 6db3d628..c91f4074 100644 --- a/pkg/openfeature/event_executor.go +++ b/pkg/openfeature/event_executor.go @@ -163,23 +163,35 @@ func (e *eventExecutor) removeClientHandler(name string, t EventType, c EventCal e.scopedRegistry[name].callbacks[t] = entrySlice } -// emitOnRegistration fulfils the spec requirement to fire ready events if associated provider is ready -func (e *eventExecutor) emitOnRegistration(providerReference providerReference, t EventType, c EventCallback) { - if t != ProviderReady { - return - } - +// emitOnRegistration fulfils the spec requirement to fire events if the +// event type and the state of the associated provider are compatible. +func (e *eventExecutor) emitOnRegistration( + providerReference providerReference, + eventType EventType, + callback EventCallback, +) { s, ok := (providerReference.featureProvider).(StateHandler) if !ok { // not a state handler, hence ignore state emitting return } - if s.Status() == ReadyState { - (*c)(EventDetails{ - providerName: (providerReference.featureProvider).Metadata().Name, + state := s.Status() + + var message string + if state == ReadyState && eventType == ProviderReady { + message = "provider is in ready state" + } else if state == ErrorState && eventType == ProviderError { + message = "provider is in error state" + } else if state == StaleState && eventType == ProviderStale { + message = "provider is in stale state" + } + + if message != "" { + (*callback)(EventDetails{ + providerName: providerReference.featureProvider.Metadata().Name, ProviderEventDetails: ProviderEventDetails{ - Message: "provider is at ready state", + Message: message, }, }) } diff --git a/pkg/openfeature/event_executor_test.go b/pkg/openfeature/event_executor_test.go index 12220df7..17300546 100644 --- a/pkg/openfeature/event_executor_test.go +++ b/pkg/openfeature/event_executor_test.go @@ -821,6 +821,208 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { }) } +// Requirement 5.3.3, Spec version 0.7.0: Handlers attached after the +// provider is already in the associated state, MUST run immediately +func TestEventHandler_HandlersRunImmediately(t *testing.T) { + t.Run("ready handler runs when provider error", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: ReadyState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 1) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderReady, &callback) + + select { + case <-rsp: + break + case <-time.After(200 * time.Millisecond): + t.Errorf("timed out waiting for ready state callback") + } + }) + + t.Run("error handler runs when provider error", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: ErrorState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 1) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderError, &callback) + + select { + case <-rsp: + break + case <-time.After(200 * time.Millisecond): + t.Errorf("timed out waiting for ready state callback") + } + }) + + t.Run("stale handler runs when provider stale", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: StaleState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 1) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderStale, &callback) + + select { + case <-rsp: + break + case <-time.After(200 * time.Millisecond): + t.Errorf("timed out waiting for ready state callback") + } + }) + + t.Run("non-ready handler does not run when provider ready", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: ReadyState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 3) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderError, &callback) + AddHandler(ProviderStale, &callback) + AddHandler(ProviderConfigChange, &callback) + + select { + case <-rsp: + t.Errorf("event must not emit for this handler") + case <-time.After(200 * time.Millisecond): + break + } + }) + + t.Run("non-error handler does not run when provider error", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: ErrorState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 3) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderReady, &callback) + AddHandler(ProviderStale, &callback) + AddHandler(ProviderConfigChange, &callback) + + select { + case <-rsp: + t.Errorf("event must not emit for this handler") + case <-time.After(200 * time.Millisecond): + break + } + }) + + t.Run("non-stale handler does not run when provider stale", func(t *testing.T) { + defer t.Cleanup(initSingleton) + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + State: StaleState, + }, + } + + if err := SetProvider(provider); err != nil { + t.Fatal(err) + } + + rsp := make(chan EventDetails, 3) + callback := func(e EventDetails) { + rsp <- e + } + + AddHandler(ProviderReady, &callback) + AddHandler(ProviderError, &callback) + AddHandler(ProviderConfigChange, &callback) + + select { + case <-rsp: + t.Errorf("event must not emit for this handler") + case <-time.After(200 * time.Millisecond): + break + } + }) +} + // non-spec bound validations func TestEventHandler_multiSubs(t *testing.T) { diff --git a/pkg/openfeature/provider.go b/pkg/openfeature/provider.go index 9c068e00..135ba85c 100644 --- a/pkg/openfeature/provider.go +++ b/pkg/openfeature/provider.go @@ -26,6 +26,7 @@ const ( NotReadyState State = "NOT_READY" ReadyState State = "READY" ErrorState State = "ERROR" + StaleState State = "STALE" ProviderReady EventType = "PROVIDER_READY" ProviderConfigChange EventType = "PROVIDER_CONFIGURATION_CHANGED" From 617ef2b92c70e4eb92a7dfef14f065a845d79470 Mon Sep 17 00:00:00 2001 From: Craig Pastro Date: Wed, 13 Sep 2023 14:02:55 -0700 Subject: [PATCH 2/2] Update based on feedback Signed-off-by: Craig Pastro --- pkg/openfeature/event_executor_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/openfeature/event_executor_test.go b/pkg/openfeature/event_executor_test.go index 17300546..a176551a 100644 --- a/pkg/openfeature/event_executor_test.go +++ b/pkg/openfeature/event_executor_test.go @@ -824,7 +824,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { // Requirement 5.3.3, Spec version 0.7.0: Handlers attached after the // provider is already in the associated state, MUST run immediately func TestEventHandler_HandlersRunImmediately(t *testing.T) { - t.Run("ready handler runs when provider error", func(t *testing.T) { + t.Run("ready handler runs when provider ready", func(t *testing.T) { defer t.Cleanup(initSingleton) provider := struct { @@ -852,7 +852,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { case <-rsp: break case <-time.After(200 * time.Millisecond): - t.Errorf("timed out waiting for ready state callback") + t.Errorf("timed out waiting for callback") } }) @@ -884,7 +884,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { case <-rsp: break case <-time.After(200 * time.Millisecond): - t.Errorf("timed out waiting for ready state callback") + t.Errorf("timed out waiting for callback") } }) @@ -916,7 +916,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { case <-rsp: break case <-time.After(200 * time.Millisecond): - t.Errorf("timed out waiting for ready state callback") + t.Errorf("timed out waiting for callback") } })