From f06532e6a621ce2baa574592a697c846e5d9e0b5 Mon Sep 17 00:00:00 2001 From: Sohail Hussain Date: Thu, 19 Dec 2019 11:56:22 -0800 Subject: [PATCH 1/8] remote notification on every call and avoid hardcoded datafile notification --- pkg/config/polling_manager.go | 10 ++- pkg/config/polling_manager_test.go | 106 +++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/pkg/config/polling_manager.go b/pkg/config/polling_manager.go index 2a2d6ae28..b0a9c9753 100644 --- a/pkg/config/polling_manager.go +++ b/pkg/config/polling_manager.go @@ -96,6 +96,7 @@ func WithInitialDatafile(datafile []byte) OptionFunc { // SyncConfig gets current datafile and updates projectConfig func (cm *PollingProjectConfigManager) SyncConfig(datafile []byte) { + initDatafile := datafile var e error var code int var respHeaders http.Header @@ -159,7 +160,7 @@ func (cm *PollingProjectConfigManager) SyncConfig(datafile []byte) { cm.projectConfig = projectConfig closeMutex(nil) - if cm.notificationCenter != nil { + if cm.notificationCenter != nil && len(initDatafile) == 0 { projectConfigUpdateNotification := notification.ProjectConfigUpdateNotification{ Type: notification.ProjectConfigUpdate, Revision: cm.projectConfig.GetRevision(), @@ -173,6 +174,8 @@ func (cm *PollingProjectConfigManager) SyncConfig(datafile []byte) { // Start starts the polling func (cm *PollingProjectConfigManager) Start(ctx context.Context) { cmLogger.Debug("Polling Config Manager Initiated") + cm.SyncConfig([]byte{}) + t := time.NewTicker(cm.pollingInterval) for { select { @@ -201,7 +204,10 @@ func NewPollingProjectConfigManager(sdkKey string, pollingMangerOptions ...Optio } initDatafile := pollingProjectConfigManager.initDatafile - pollingProjectConfigManager.SyncConfig(initDatafile) // initial poll + if len(initDatafile) > 0 { + pollingProjectConfigManager.SyncConfig(initDatafile) // initial poll + } + return &pollingProjectConfigManager } diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index aa5ac507a..0f916ecc5 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -44,6 +44,18 @@ func newExecGroup() *utils.ExecGroup { return utils.NewExecGroup(context.Background()) } +// assertion method to wait for config or err for a specified period of time. +func waitForConfigOrCancelTimeout(t *testing.T, configManager ProjectConfigManager, checkError bool) { + assert.Eventually(t, func() bool { + config, err := configManager.GetConfig() + if checkError { + return err != nil + } else { + return config != nil + } + }, 500*time.Millisecond, 10*time.Millisecond) +} + func TestNewPollingProjectConfigManagerWithOptions(t *testing.T) { mockDatafile := []byte(`{"revision":"42"}`) @@ -57,6 +69,8 @@ func TestNewPollingProjectConfigManagerWithOptions(t *testing.T) { eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + + waitForConfigOrCancelTimeout(t, configManager, false) mockRequester.AssertExpectations(t) actual, err := configManager.GetConfig() @@ -78,10 +92,11 @@ func TestNewPollingProjectConfigManagerWithNull(t *testing.T) { eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + + waitForConfigOrCancelTimeout(t, configManager, true) + mockRequester.AssertExpectations(t) - _, err := configManager.GetConfig() - assert.NotNil(t, err) } func TestNewPollingProjectConfigManagerWithSimilarDatafileRevisions(t *testing.T) { @@ -96,6 +111,9 @@ func TestNewPollingProjectConfigManagerWithSimilarDatafileRevisions(t *testing.T eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + + waitForConfigOrCancelTimeout(t, configManager, false) + mockRequester.AssertExpectations(t) actual, err := configManager.GetConfig() @@ -125,6 +143,8 @@ func TestNewPollingProjectConfigManagerWithLastModifiedDates(t *testing.T) { configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + waitForConfigOrCancelTimeout(t, configManager, false) + // Fetch valid config actual, err := configManager.GetConfig() assert.Nil(t, err) @@ -154,6 +174,9 @@ func TestNewPollingProjectConfigManagerWithDifferentDatafileRevisions(t *testing eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + + waitForConfigOrCancelTimeout(t, configManager, false) + mockRequester.AssertExpectations(t) actual, err := configManager.GetConfig() @@ -181,6 +204,9 @@ func TestNewPollingProjectConfigManagerWithErrorHandling(t *testing.T) { eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) eg.Go(configManager.Start) + + waitForConfigOrCancelTimeout(t, configManager, true) + mockRequester.AssertExpectations(t) actual, err := configManager.GetConfig() // polling for bad file @@ -199,7 +225,7 @@ func TestNewPollingProjectConfigManagerWithErrorHandling(t *testing.T) { assert.Equal(t, projectConfig2, actual) } -func TestNewPollingProjectConfigManagerOnDecision(t *testing.T) { +func TestNewPollingProjectConfigManagerOnConfigUpdate(t *testing.T) { mockDatafile1 := []byte(`{"revision":"42","botFiltering":true}`) mockDatafile2 := []byte(`{"revision":"43","botFiltering":false}`) @@ -211,13 +237,16 @@ func TestNewPollingProjectConfigManagerOnDecision(t *testing.T) { eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) - eg.Go(configManager.Start) var numberOfCalls = 0 callback := func(notification notification.ProjectConfigUpdateNotification) { numberOfCalls++ } id, _ := configManager.OnProjectConfigUpdate(callback) + + eg.Go(configManager.Start) + waitForConfigOrCancelTimeout(t, configManager, false) + mockRequester.AssertExpectations(t) actual, err := configManager.GetConfig() @@ -230,7 +259,7 @@ func TestNewPollingProjectConfigManagerOnDecision(t *testing.T) { assert.NotNil(t, actual) assert.NotEqual(t, id, 0) - assert.Equal(t, numberOfCalls, 1) + assert.Equal(t, numberOfCalls, 2) err = configManager.RemoveOnProjectConfigUpdate(id) assert.Nil(t, err) @@ -239,6 +268,73 @@ func TestNewPollingProjectConfigManagerOnDecision(t *testing.T) { assert.Nil(t, err) } +func TestNewPollingProjectConfigManagerHardcodedDatafile(t *testing.T) { + mockDatafile1 := []byte(`{"revision":"42"}`) + mockDatafile2 := []byte(`{"revision":"43"}`) + sdkKey := "test_sdk_key" + + mockRequester := new(MockRequester) + mockRequester.On("Get", []utils.Header(nil)).Return(mockDatafile2, http.Header{}, http.StatusOK, nil) + + configManager := NewPollingProjectConfigManager(sdkKey, WithInitialDatafile(mockDatafile1)) + config, err := configManager.GetConfig() + + mockRequester.AssertNotCalled(t, "Get") + assert.Nil(t, err) + assert.NotNil(t, config) + assert.Equal(t, "42", config.GetRevision()) +} + +func TestNewPollingProjectConfigManagerPullImmediatelyOnStart(t *testing.T) { + + mockDatafile1 := []byte(`{"revision":"44"}`) // remote + mockDatafile2 := []byte(`{"revision":"43"}`) // hardcoded + + mockRequester := new(MockRequester) + mockRequester.On("Get", []utils.Header(nil)).Return(mockDatafile1, http.Header{}, http.StatusOK, nil) + + // Test we fetch using requester + sdkKey := "test_sdk_key" + + configManager := NewPollingProjectConfigManager(sdkKey, + WithRequester(mockRequester), + WithInitialDatafile(mockDatafile2), + // want to make sure regardless of any polling interval, syncconfig should be called immediately + WithPollingInterval(10*time.Second)) + + config, err := configManager.GetConfig() + + numberOfCalls := 0 + + // hardcoded datafile assertion + assert.Nil(t, err) + assert.NotNil(t, config) + assert.Equal(t, "43", config.GetRevision()) + mockRequester.AssertNotCalled(t, "Get") + + callback := func(notification notification.ProjectConfigUpdateNotification) { + numberOfCalls++ + } + + configManager.OnProjectConfigUpdate(callback) + + eg := newExecGroup() + eg.Go(configManager.Start) + + assert.Eventually(t, func() bool { + return numberOfCalls == 1 + }, 1500*time.Millisecond, 10*time.Millisecond) + + mockRequester.AssertExpectations(t) + + remoteConfig, err := configManager.GetConfig() + assert.Nil(t, err) + assert.NotNil(t, remoteConfig) + assert.Equal(t, "44", remoteConfig.GetRevision()) + + eg.TerminateAndWait() // just sending signal and improving coverage +} + func TestPollingInterval(t *testing.T) { sdkKey := "test_sdk_key" From 9d0c4bb98db230a3e2dac7119e62b424d9bb52fc Mon Sep 17 00:00:00 2001 From: Sohail Hussain Date: Thu, 19 Dec 2019 16:08:31 -0800 Subject: [PATCH 2/8] add lock and unlock to avoid race condition. --- pkg/config/polling_manager_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index 0f916ecc5..97275c544 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -19,6 +19,7 @@ package config import ( "context" "net/http" + "sync" "testing" "time" @@ -259,7 +260,7 @@ func TestNewPollingProjectConfigManagerOnConfigUpdate(t *testing.T) { assert.NotNil(t, actual) assert.NotEqual(t, id, 0) - assert.Equal(t, numberOfCalls, 2) + assert.Equal(t, numberOfCalls, 1) err = configManager.RemoveOnProjectConfigUpdate(id) assert.Nil(t, err) @@ -286,7 +287,7 @@ func TestNewPollingProjectConfigManagerHardcodedDatafile(t *testing.T) { } func TestNewPollingProjectConfigManagerPullImmediatelyOnStart(t *testing.T) { - + m := sync.RWMutex{} mockDatafile1 := []byte(`{"revision":"44"}`) // remote mockDatafile2 := []byte(`{"revision":"43"}`) // hardcoded @@ -313,6 +314,8 @@ func TestNewPollingProjectConfigManagerPullImmediatelyOnStart(t *testing.T) { mockRequester.AssertNotCalled(t, "Get") callback := func(notification notification.ProjectConfigUpdateNotification) { + m.Lock() + defer m.Unlock() numberOfCalls++ } @@ -322,6 +325,8 @@ func TestNewPollingProjectConfigManagerPullImmediatelyOnStart(t *testing.T) { eg.Go(configManager.Start) assert.Eventually(t, func() bool { + m.Lock() + defer m.Unlock() return numberOfCalls == 1 }, 1500*time.Millisecond, 10*time.Millisecond) From 40fb914f1efd58917ad2a5665677399d342c2384 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 20 Dec 2019 12:36:41 +0500 Subject: [PATCH 3/8] refactored waitForConfigOrCancelTimeout. --- pkg/config/polling_manager_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index 97275c544..d53a6c3ca 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -48,12 +48,8 @@ func newExecGroup() *utils.ExecGroup { // assertion method to wait for config or err for a specified period of time. func waitForConfigOrCancelTimeout(t *testing.T, configManager ProjectConfigManager, checkError bool) { assert.Eventually(t, func() bool { - config, err := configManager.GetConfig() - if checkError { - return err != nil - } else { - return config != nil - } + _, err := configManager.GetConfig() + return (err != nil) == checkError }, 500*time.Millisecond, 10*time.Millisecond) } From dbaac0a1e0c0bde3267640dec94a1256e6e95a35 Mon Sep 17 00:00:00 2001 From: Pawel Szczodruch Date: Fri, 20 Dec 2019 11:18:44 -0800 Subject: [PATCH 4/8] fix race condition --- pkg/config/polling_manager.go | 2 +- pkg/config/polling_manager_test.go | 7 ++++++- pkg/notification/manager.go | 8 ++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/config/polling_manager.go b/pkg/config/polling_manager.go index b0a9c9753..203dcd319 100644 --- a/pkg/config/polling_manager.go +++ b/pkg/config/polling_manager.go @@ -163,7 +163,7 @@ func (cm *PollingProjectConfigManager) SyncConfig(datafile []byte) { if cm.notificationCenter != nil && len(initDatafile) == 0 { projectConfigUpdateNotification := notification.ProjectConfigUpdateNotification{ Type: notification.ProjectConfigUpdate, - Revision: cm.projectConfig.GetRevision(), + Revision: projectConfig.GetRevision(), } if err = cm.notificationCenter.Send(notification.ProjectConfigUpdate, projectConfigUpdateNotification); err != nil { cmLogger.Warning("Problem with sending notification") diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index d53a6c3ca..1c34fb2ed 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -234,9 +234,11 @@ func TestNewPollingProjectConfigManagerOnConfigUpdate(t *testing.T) { eg := newExecGroup() configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) - + m := sync.RWMutex{} var numberOfCalls = 0 callback := func(notification notification.ProjectConfigUpdateNotification) { + m.Lock() + defer m.Unlock() numberOfCalls++ } id, _ := configManager.OnProjectConfigUpdate(callback) @@ -256,7 +258,10 @@ func TestNewPollingProjectConfigManagerOnConfigUpdate(t *testing.T) { assert.NotNil(t, actual) assert.NotEqual(t, id, 0) + + m.Lock() assert.Equal(t, numberOfCalls, 1) + m.Unlock() err = configManager.RemoveOnProjectConfigUpdate(id) assert.Nil(t, err) diff --git a/pkg/notification/manager.go b/pkg/notification/manager.go index b95fef810..b6799be91 100644 --- a/pkg/notification/manager.go +++ b/pkg/notification/manager.go @@ -19,6 +19,7 @@ package notification import ( "fmt" + "sync" "sync/atomic" "github.com/optimizely/go-sdk/pkg/logging" @@ -37,6 +38,7 @@ type Manager interface { type AtomicManager struct { handlers map[uint32]func(interface{}) counter uint32 + lock sync.Mutex } // NewAtomicManager creates a new instance of the atomic manager @@ -49,6 +51,8 @@ func NewAtomicManager() *AtomicManager { // Add adds the given handler func (am *AtomicManager) Add(newHandler func(interface{})) (int, error) { atomic.AddUint32(&am.counter, 1) + am.lock.Lock() + defer am.lock.Unlock() am.handlers[am.counter] = newHandler return int(am.counter), nil } @@ -56,6 +60,8 @@ func (am *AtomicManager) Add(newHandler func(interface{})) (int, error) { // Remove removes handler with the given id func (am *AtomicManager) Remove(id int) { handlerID := uint32(id) + am.lock.Lock() + defer am.lock.Unlock() if _, ok := am.handlers[handlerID]; ok { delete(am.handlers, handlerID) return @@ -66,6 +72,8 @@ func (am *AtomicManager) Remove(id int) { // Send sends the notification to the registered handlers func (am *AtomicManager) Send(notification interface{}) { + am.lock.Lock() + defer am.lock.Unlock() for _, handler := range am.handlers { handler(notification) } From 15a8df233064a019442c4867f6d67dc77556e5b2 Mon Sep 17 00:00:00 2001 From: Pawel Szczodruch Date: Fri, 20 Dec 2019 11:42:49 -0800 Subject: [PATCH 5/8] changed the test a little --- pkg/config/polling_manager_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index 4320b45a3..b2a621c87 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -190,15 +190,14 @@ func TestPollingGetOptimizelyConfig(t *testing.T) { mockDatafile1 := []byte(`{"revision":"42","botFiltering":true}`) mockDatafile2 := []byte(`{"revision":"43","botFiltering":false}`) mockRequester := new(MockRequester) - mockRequester.On("Get", []utils.Header(nil)).Return(mockDatafile1, http.Header{}, http.StatusOK, nil) + mockRequester.On("Get", []utils.Header(nil)).Return([]byte{}, http.Header{}, http.StatusOK, nil) // Test we fetch using requester sdkKey := "test_sdk_key" eg := newExecGroup() - configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester)) + configManager := NewPollingProjectConfigManager(sdkKey, WithRequester(mockRequester), WithInitialDatafile(mockDatafile1)) eg.Go(configManager.Start) - mockRequester.AssertExpectations(t) assert.Nil(t, configManager.optimizelyConfig) @@ -212,6 +211,7 @@ func TestPollingGetOptimizelyConfig(t *testing.T) { configManager.SyncConfig(mockDatafile2) optimizelyConfig = configManager.GetOptimizelyConfig() assert.Equal(t, "43", optimizelyConfig.Revision) + mockRequester.AssertExpectations(t) } From 9ad9dbb0e5ccd059042aa46ba9df83ae0cc827cd Mon Sep 17 00:00:00 2001 From: Pawel Szczodruch Date: Fri, 20 Dec 2019 11:49:14 -0800 Subject: [PATCH 6/8] commenting for now --- pkg/config/polling_manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index b2a621c87..416834c7c 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -211,7 +211,7 @@ func TestPollingGetOptimizelyConfig(t *testing.T) { configManager.SyncConfig(mockDatafile2) optimizelyConfig = configManager.GetOptimizelyConfig() assert.Equal(t, "43", optimizelyConfig.Revision) - mockRequester.AssertExpectations(t) + //mockRequester.AssertExpectations(t) } From 8465b8a7d253a3f331894ca4bc74dc74fd675012 Mon Sep 17 00:00:00 2001 From: Sohail Hussain Date: Fri, 20 Dec 2019 13:45:34 -0800 Subject: [PATCH 7/8] corrected getoptimizelyconfig testcase --- pkg/config/polling_manager_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index 416834c7c..5b9ddd38d 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -190,7 +190,7 @@ func TestPollingGetOptimizelyConfig(t *testing.T) { mockDatafile1 := []byte(`{"revision":"42","botFiltering":true}`) mockDatafile2 := []byte(`{"revision":"43","botFiltering":false}`) mockRequester := new(MockRequester) - mockRequester.On("Get", []utils.Header(nil)).Return([]byte{}, http.Header{}, http.StatusOK, nil) + mockRequester.On("Get", []utils.Header(nil)).Return([]byte(mockDatafile2), http.Header{}, http.StatusOK, nil) // Test we fetch using requester sdkKey := "test_sdk_key" @@ -208,11 +208,14 @@ func TestPollingGetOptimizelyConfig(t *testing.T) { assert.Equal(t, "42", optimizelyConfig.Revision) - configManager.SyncConfig(mockDatafile2) + assert.Eventually(t, func() bool { + config, _ := configManager.GetConfig() + return config.GetRevision() == "43" + }, time.Second, 10*time.Millisecond) + optimizelyConfig = configManager.GetOptimizelyConfig() assert.Equal(t, "43", optimizelyConfig.Revision) - //mockRequester.AssertExpectations(t) - + mockRequester.AssertExpectations(t) } func TestNewPollingProjectConfigManagerWithErrorHandling(t *testing.T) { From a3731540475f7cfe35f7f3d986f21c38a4005d6f Mon Sep 17 00:00:00 2001 From: Sohail Hussain Date: Thu, 26 Dec 2019 10:55:43 -0800 Subject: [PATCH 8/8] requester added in unit test. --- pkg/config/polling_manager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/config/polling_manager_test.go b/pkg/config/polling_manager_test.go index 5b9ddd38d..5c941445d 100644 --- a/pkg/config/polling_manager_test.go +++ b/pkg/config/polling_manager_test.go @@ -310,7 +310,7 @@ func TestNewPollingProjectConfigManagerHardcodedDatafile(t *testing.T) { mockRequester := new(MockRequester) mockRequester.On("Get", []utils.Header(nil)).Return(mockDatafile2, http.Header{}, http.StatusOK, nil) - configManager := NewPollingProjectConfigManager(sdkKey, WithInitialDatafile(mockDatafile1)) + configManager := NewPollingProjectConfigManager(sdkKey, WithInitialDatafile(mockDatafile1), WithRequester(mockRequester)) config, err := configManager.GetConfig() mockRequester.AssertNotCalled(t, "Get")