From 6f71fe40505f07fc241953ee9da8ea4edc6e4d35 Mon Sep 17 00:00:00 2001 From: Kavindu Dodanduwa Date: Wed, 7 Feb 2024 11:16:47 -0800 Subject: [PATCH] feat: blocking provider mutator (#251) * introduce wait apis and improve existing tests Signed-off-by: Kavindu Dodanduwa * tests for new apis Signed-off-by: Kavindu Dodanduwa * improve race conditions Signed-off-by: Kavindu Dodanduwa * return error for synchronous provider registrations Signed-off-by: Kavindu Dodanduwa --------- Signed-off-by: Kavindu Dodanduwa --- openfeature/api.go | 112 ++++++++++++++--------- openfeature/event_executor.go | 2 +- openfeature/event_executor_test.go | 29 +++--- openfeature/openfeature.go | 16 +++- openfeature/openfeature_test.go | 141 +++++++++++++++++++++++++++++ 5 files changed, 240 insertions(+), 60 deletions(-) diff --git a/openfeature/api.go b/openfeature/api.go index c02fed30..155c5e2b 100644 --- a/openfeature/api.go +++ b/openfeature/api.go @@ -2,6 +2,7 @@ package openfeature import ( "errors" + "fmt" "sync" "github.com/go-logr/logr" @@ -14,7 +15,7 @@ type evaluationAPI struct { defaultProvider FeatureProvider namedProviders map[string]FeatureProvider hks []Hook - evalCtx EvaluationContext + apiCtx EvaluationContext logger logr.Logger mu sync.RWMutex eventExecutor *eventExecutor @@ -28,15 +29,16 @@ func newEvaluationAPI() evaluationAPI { defaultProvider: NoopProvider{}, namedProviders: map[string]FeatureProvider{}, hks: []Hook{}, - evalCtx: EvaluationContext{}, + apiCtx: EvaluationContext{}, logger: logger, mu: sync.RWMutex{}, eventExecutor: newEventExecutor(logger), } } -// setProvider sets the default FeatureProvider of the evaluationAPI. Returns an error if FeatureProvider is nil -func (api *evaluationAPI) setProvider(provider FeatureProvider) error { +// setProvider sets the default FeatureProvider of the evaluationAPI. +// Returns an error if provider registration cause an error +func (api *evaluationAPI) setProvider(provider FeatureProvider, async bool) error { api.mu.Lock() defer api.mu.Unlock() @@ -44,13 +46,15 @@ func (api *evaluationAPI) setProvider(provider FeatureProvider) error { return errors.New("default provider cannot be set to nil") } - // Initialize new default provider and shutdown the old one - // Provider update must be non-blocking, hence initialization & shutdown happens concurrently oldProvider := api.defaultProvider api.defaultProvider = provider - api.initNewAndShutdownOld(provider, oldProvider) - err := api.eventExecutor.registerDefaultProvider(provider) + err := api.initNewAndShutdownOld(provider, oldProvider, async) + if err != nil { + return err + } + + err = api.eventExecutor.registerDefaultProvider(provider) if err != nil { return err } @@ -67,7 +71,7 @@ func (api *evaluationAPI) getProvider() FeatureProvider { } // setProvider sets a provider with client name. Returns an error if FeatureProvider is nil -func (api *evaluationAPI) setNamedProvider(clientName string, provider FeatureProvider) error { +func (api *evaluationAPI) setNamedProvider(clientName string, provider FeatureProvider, async bool) error { api.mu.Lock() defer api.mu.Unlock() @@ -80,8 +84,12 @@ func (api *evaluationAPI) setNamedProvider(clientName string, provider FeaturePr oldProvider := api.namedProviders[clientName] api.namedProviders[clientName] = provider - api.initNewAndShutdownOld(provider, oldProvider) - err := api.eventExecutor.registerNamedEventingProvider(clientName, provider) + err := api.initNewAndShutdownOld(provider, oldProvider, async) + if err != nil { + return err + } + + err = api.eventExecutor.registerNamedEventingProvider(clientName, provider) if err != nil { return err } @@ -97,11 +105,11 @@ func (api *evaluationAPI) getNamedProviders() map[string]FeatureProvider { return api.namedProviders } -func (api *evaluationAPI) setEvaluationContext(evalCtx EvaluationContext) { +func (api *evaluationAPI) setEvaluationContext(apiCtx EvaluationContext) { api.mu.Lock() defer api.mu.Unlock() - api.evalCtx = evalCtx + api.apiCtx = apiCtx } func (api *evaluationAPI) setLogger(l logr.Logger) { @@ -163,52 +171,68 @@ func (api *evaluationAPI) forTransaction(clientName string) (FeatureProvider, [] provider = api.defaultProvider } - return provider, api.hks, api.evalCtx + return provider, api.hks, api.apiCtx } // initNewAndShutdownOld is a helper to initialise new FeatureProvider and shutdown the old FeatureProvider. -// Operations happen concurrently. -func (api *evaluationAPI) initNewAndShutdownOld(newProvider FeatureProvider, oldProvider FeatureProvider) { - v, ok := newProvider.(StateHandler) - if ok && v.Status() == NotReadyState { - go func(provider FeatureProvider, stateHandler StateHandler, evalCtx EvaluationContext, eventChan chan eventPayload) { - err := stateHandler.Init(evalCtx) - // emit ready/error event once initialization is complete - if err != nil { - eventChan <- eventPayload{ - Event{ - ProviderName: provider.Metadata().Name, - EventType: ProviderError, - ProviderEventDetails: ProviderEventDetails{}, - }, provider, - } - } else { - eventChan <- eventPayload{ - Event{ - ProviderName: provider.Metadata().Name, - EventType: ProviderReady, - ProviderEventDetails: ProviderEventDetails{}, - }, provider, - } - } - }(newProvider, v, api.evalCtx, api.eventExecutor.eventChan) - } - - v, ok = oldProvider.(StateHandler) +func (api *evaluationAPI) initNewAndShutdownOld(newProvider FeatureProvider, oldProvider FeatureProvider, async bool) error { + if async { + go func(executor *eventExecutor, ctx EvaluationContext) { + // for async initialization, error is conveyed as an event + event, _ := initializer(newProvider, ctx) + executor.triggerEvent(event, newProvider) + }(api.eventExecutor, api.apiCtx) + } else { + event, err := initializer(newProvider, api.apiCtx) + api.eventExecutor.triggerEvent(event, newProvider) + if err != nil { + return err + } + } + + v, ok := oldProvider.(StateHandler) // oldProvider can be nil or without state handling capability if oldProvider == nil || !ok { - return + return nil } // check for multiple bindings if oldProvider == api.defaultProvider || contains(oldProvider, maps.Values(api.namedProviders)) { - return + return nil } go func(forShutdown StateHandler) { forShutdown.Shutdown() }(v) + + return nil +} + +// initializer is a helper to execute provider initialization and generate appropriate event for the initialization +// It also returns an error if the initialization resulted in an error +func initializer(provider FeatureProvider, apiCtx EvaluationContext) (Event, error) { + var event = Event{ + ProviderName: provider.Metadata().Name, + EventType: ProviderReady, + ProviderEventDetails: ProviderEventDetails{ + Message: "Provider initialization successful", + }, + } + + handler, ok := provider.(StateHandler) + if !ok { + // Note - a provider without state handling capability can be assumed to be ready immediately. + return event, nil + } + + err := handler.Init(apiCtx) + if err != nil { + event.EventType = ProviderError + event.Message = fmt.Sprintf("Provider initialization error, %v", err) + } + + return event, err } func contains(provider FeatureProvider, in []FeatureProvider) bool { diff --git a/openfeature/event_executor.go b/openfeature/event_executor.go index 8644e431..b3df6cb3 100644 --- a/openfeature/event_executor.go +++ b/openfeature/event_executor.go @@ -327,7 +327,7 @@ func (e *eventExecutor) triggerEvent(event Event, handler FeatureProvider) { } } - if e.defaultProviderReference.featureProvider != handler { + if !reflect.DeepEqual(e.defaultProviderReference.featureProvider, handler) { return } diff --git a/openfeature/event_executor_test.go b/openfeature/event_executor_test.go index ed41e3af..d4783e99 100644 --- a/openfeature/event_executor_test.go +++ b/openfeature/event_executor_test.go @@ -82,7 +82,8 @@ func TestEventHandler_Eventing(t *testing.T) { rsp <- details } - AddHandler(ProviderReady, &callBack) + eventType := ProviderConfigChange + AddHandler(eventType, &callBack) fCh := []string{"flagA"} meta := map[string]interface{}{ @@ -91,7 +92,7 @@ func TestEventHandler_Eventing(t *testing.T) { // trigger event from provider implementation eventingImpl.Invoke(Event{ - EventType: ProviderReady, + EventType: eventType, ProviderEventDetails: ProviderEventDetails{ Message: "ReadyMessage", FlagChanges: fCh, @@ -139,7 +140,7 @@ func TestEventHandler_Eventing(t *testing.T) { // associated to client name associatedName := "providerForClient" - err := SetNamedProvider(associatedName, eventingProvider) + err := SetNamedProviderAndWait(associatedName, eventingProvider) if err != nil { t.Fatal(err) } @@ -214,13 +215,13 @@ func TestEventHandler_clientAssociation(t *testing.T) { } // default provider - err := SetProvider(defaultProvider) + err := SetProviderAndWait(defaultProvider) if err != nil { t.Fatal(err) } // named provider(associated to name someClient) - err = SetNamedProvider("someClient", struct { + err = SetNamedProviderAndWait("someClient", struct { FeatureProvider EventHandler }{ @@ -273,7 +274,7 @@ func TestEventHandler_ErrorHandling(t *testing.T) { eventing, } - errorCallback := func(e EventDetails) { + failingCallback := func(e EventDetails) { panic("callback panic") } @@ -292,9 +293,11 @@ func TestEventHandler_ErrorHandling(t *testing.T) { t.Fatal(err) } + successEventType := ProviderStale + // api level handlers - AddHandler(ProviderReady, &errorCallback) - AddHandler(ProviderReady, &successAPICallback) + AddHandler(ProviderConfigChange, &failingCallback) + AddHandler(successEventType, &successAPICallback) // provider association providerName := "providerA" @@ -302,14 +305,14 @@ func TestEventHandler_ErrorHandling(t *testing.T) { client := NewClient(providerName) // client level handlers - client.AddHandler(ProviderReady, &errorCallback) - client.AddHandler(ProviderReady, &successClientCallback) + client.AddHandler(ProviderConfigChange, &failingCallback) + client.AddHandler(successEventType, &successClientCallback) // trigger events manually go func() { eventing.Invoke(Event{ ProviderName: providerName, - EventType: ProviderReady, + EventType: successEventType, ProviderEventDetails: ProviderEventDetails{}, }) }() @@ -967,7 +970,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { }, } - if err := SetProvider(provider); err != nil { + if err := SetProviderAndWait(provider); err != nil { t.Fatal(err) } @@ -1001,7 +1004,7 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) { }, } - if err := SetProvider(provider); err != nil { + if err := SetProviderAndWait(provider); err != nil { t.Fatal(err) } diff --git a/openfeature/openfeature.go b/openfeature/openfeature.go index cbf5b8c7..40cf61a9 100644 --- a/openfeature/openfeature.go +++ b/openfeature/openfeature.go @@ -20,13 +20,25 @@ func initSingleton() { // SetProvider sets the default provider. Provider initialization is asynchronous and status can be checked from // provider status func SetProvider(provider FeatureProvider) error { - return api.setProvider(provider) + return api.setProvider(provider, true) +} + +// SetProviderAndWait sets the default provider and waits for its initialization. +// Returns an error if initialization cause error +func SetProviderAndWait(provider FeatureProvider) error { + return api.setProvider(provider, false) } // SetNamedProvider sets a provider mapped to the given Client name. Provider initialization is asynchronous and // status can be checked from provider status func SetNamedProvider(clientName string, provider FeatureProvider) error { - return api.setNamedProvider(clientName, provider) + return api.setNamedProvider(clientName, provider, true) +} + +// SetNamedProviderAndWait sets a provider mapped to the given Client name and waits for its initialization. +// Returns an error if initialization cause error +func SetNamedProviderAndWait(clientName string, provider FeatureProvider) error { + return api.setNamedProvider(clientName, provider, false) } // SetEvaluationContext sets the global evaluation context. diff --git a/openfeature/openfeature_test.go b/openfeature/openfeature_test.go index c98b68b7..7fa8d0c2 100644 --- a/openfeature/openfeature_test.go +++ b/openfeature/openfeature_test.go @@ -1,6 +1,7 @@ package openfeature import ( + "errors" "reflect" "testing" "time" @@ -252,6 +253,146 @@ func TestRequirement_1_1_2_3(t *testing.T) { }) } +// The API SHOULD provide functions to set a provider and wait for the initialize function to return or throw. +func TestRequirement_1_1_2_4(t *testing.T) { + defer t.Cleanup(initSingleton) + + t.Run("default provider", func(t *testing.T) { + // given - a provider with state handling capability, with substantial initializing delay + var initialized = false + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + initF: func(e EvaluationContext) error { + <-time.After(200 * time.Millisecond) + initialized = true + return nil + }, + }, + } + + // when - registered + err := SetProviderAndWait(provider) + if err != nil { + t.Fatal(err) + } + + // then - must update variable as call is blocking + if initialized != true { + t.Errorf("expected initialization, but got false") + } + }) + + t.Run("named provider", func(t *testing.T) { + // given - a provider with state handling capability, with substantial initializing delay + var initialized = false + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + initF: func(e EvaluationContext) error { + <-time.After(200 * time.Millisecond) + initialized = true + return nil + }, + }, + } + + // when - registered + err := SetNamedProviderAndWait("someName", provider) + if err != nil { + t.Fatal(err) + } + + // then - must update variable as call is blocking + if initialized != true { + t.Errorf("expected initialization, but got false") + } + }) + + t.Run("error return and eventing", func(t *testing.T) { + // given - provider with initialization error & error handlers registered + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + initF: func(e EvaluationContext) error { + <-time.After(200 * time.Millisecond) + return errors.New("some initialization error") + }, + }, + } + + errChan := make(chan EventDetails, 1) + errHandler := func(details EventDetails) { + errChan <- details + } + + AddHandler(ProviderError, &errHandler) + + // when + err := SetProviderAndWait(provider) + + // then + if err == nil { + t.Fatal("expected error to be non-nil, but got nil") + } + + var errEvent EventDetails + + select { + case <-time.After(200 * time.Millisecond): + t.Fatal("expected error event, but time out waiting for event") + case errEvent = <-errChan: + break + } + + if errEvent.Message == "" { + t.Fatal("expected non empty event message, but got empty") + } + + }) + + t.Run("async registration to validate by contradiction", func(t *testing.T) { + // given - a provider with state handling capability, with substantial initializing delay + var initialized = false + + provider := struct { + FeatureProvider + StateHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + initF: func(e EvaluationContext) error { + <-time.After(200 * time.Millisecond) + initialized = true + return nil + }, + }, + } + + // when - registered async + err := SetProvider(provider) + if err != nil { + t.Fatal(err) + } + + // then - must not update variable as registration is async + if initialized != false { + t.Errorf("expected uninitialized as async, but got true") + } + }) +} + // The `API` MUST provide a function to bind a given `provider` to one or more client `name`s. // If the client-name already has a bound provider, it is overwritten with the new mapping. func TestRequirement_1_1_3(t *testing.T) {