fix: Update built-in map-based ExperimentOverrideStore implementation to be concurrently usable (#171)

The built-in default implementation of ExperimentOverrideStore should be production ready, so it should be safe to use concurrently.

In this PR:
- The default implementation is renamed to MapExperimentOverridesStore.
- A sync.RWLock is added to MapExperimentOverridesStore.
- A SetVariation method is added for setting an override, which uses the mutex.
- The GetVariation method is updated to use the mutex.
- A new test is added that calls GetVariation and SetVariation concurrently from several goroutines.

Author: mjc1283

pkg/client/factory_test.go

@@ -151,7 +151,7 @@ func TestClientWithCustomDecisionServiceOptions(t *testing.T) {
 	factory := OptimizelyFactory{SDKKey: "1212"}
 
 	mockUserProfileService := new(MockUserProfileService)
-	mockOverrideStore := new(decision.MapOverridesStore)
+	mockOverrideStore := new(decision.MapExperimentOverridesStore)
 	optimizelyClient, err := factory.Client(
 		WithUserProfileService(mockUserProfileService),
 		WithExperimentOverrides(mockOverrideStore),

pkg/decision/composite_experiment_service_test.go

@@ -160,7 +160,7 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentService() {
 
 func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCustomOptions() {
 	mockUserProfileService := new(MockUserProfileService)
-	mockExperimentOverrideStore := new(MapOverridesStore)
+	mockExperimentOverrideStore := new(MapExperimentOverridesStore)
 	compositeExperimentService := NewCompositeExperimentService(
 		WithUserProfileService(mockUserProfileService),
 		WithOverrideStore(mockExperimentOverrideStore),

pkg/decision/experiment_override_service.go

@@ -20,6 +20,7 @@ package decision
 import (
 	"errors"
 	"fmt"
+	"sync"
 
 	"github.com/optimizely/go-sdk/pkg/decision/reasons"
 	"github.com/optimizely/go-sdk/pkg/entities"
@@ -39,17 +40,27 @@ type ExperimentOverrideStore interface {
 	GetVariation(overrideKey ExperimentOverrideKey) (string, bool)
 }
 
-// MapOverridesStore is a map-based implementation of OverrideStore
-type MapOverridesStore struct {
+// MapExperimentOverridesStore is a map-based implementation of ExperimentOverridesStore that is safe to use concurrently
+type MapExperimentOverridesStore struct {
 	overridesMap map[ExperimentOverrideKey]string
+	mutex        sync.RWMutex
 }
 
-// GetVariation returns the override associated with the given key in the map
-func (m *MapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) {
+// GetVariation returns the override variation key associated with the given user+experiment key
+func (m *MapExperimentOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) {
+	m.mutex.RLock()
 	variationKey, ok := m.overridesMap[overrideKey]
+	m.mutex.RUnlock()
 	return variationKey, ok
 }
 
+// SetVariation sets the given variation key as an override for the given user+experiment key
+func (m *MapExperimentOverridesStore) SetVariation(overrideKey ExperimentOverrideKey, variationKey string) {
+	m.mutex.Lock()
+	m.overridesMap[overrideKey] = variationKey
+	m.mutex.Unlock()
+}
+
 // ExperimentOverrideService makes a decision using an ExperimentOverridesStore
 // Implements the ExperimentService interface
 type ExperimentOverrideService struct {

pkg/decision/experiment_override_service_test.go

@@ -18,6 +18,7 @@
 package decision
 
 import (
+	"sync"
 	"testing"
 
 	"github.com/optimizely/go-sdk/pkg/decision/reasons"
@@ -28,16 +29,16 @@ import (
 type ExperimentOverrideServiceTestSuite struct {
 	suite.Suite
 	mockConfig      *mockProjectConfig
-	overrides       map[ExperimentOverrideKey]string
+	overrides       *MapExperimentOverridesStore
 	overrideService *ExperimentOverrideService
 }
 
 func (s *ExperimentOverrideServiceTestSuite) SetupTest() {
 	s.mockConfig = new(mockProjectConfig)
-	s.overrides = make(map[ExperimentOverrideKey]string)
-	s.overrideService = NewExperimentOverrideService(&MapOverridesStore{
-		overridesMap: s.overrides,
-	})
+	s.overrides = &MapExperimentOverridesStore{
+		overridesMap: make(map[ExperimentOverrideKey]string),
+	}
+	s.overrideService = NewExperimentOverrideService(s.overrides)
 }
 
 func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() {
@@ -48,7 +49,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() {
 	testUserContext := entities.UserContext{
 		ID: "test_user_1",
 	}
-	s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = testExp1111Var2222.Key
+	s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, testExp1111Var2222.Key)
 	decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext)
 	s.NoError(err)
 	s.NotNil(decision.Variation)
@@ -77,7 +78,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForExperiment() {
 		ID: "test_user_1",
 	}
 	// The decision context refers to testExp1111, but this override is for another experiment
-	s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_1"}] = testExp1113Var2224.Key
+	s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_1"}, testExp1113Var2224.Key)
 	decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext)
 	s.NoError(err)
 	s.Nil(decision.Variation)
@@ -93,7 +94,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUser() {
 		ID: "test_user_1",
 	}
 	// The user context refers to "test_user_1", but this override is for another user
-	s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}] = testExp1111Var2222.Key
+	s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}, testExp1111Var2222.Key)
 	decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext)
 	s.NoError(err)
 	s.Nil(decision.Variation)
@@ -109,7 +110,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUserOrExperiment()
 		ID: "test_user_1",
 	}
 	// This override is for both a different user and a different experiment than the ones in the contexts above
-	s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_3"}] = testExp1111Var2222.Key
+	s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_3"}, testExp1111Var2222.Key)
 	decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext)
 	s.NoError(err)
 	s.Nil(decision.Variation)
@@ -125,13 +126,69 @@ func (s *ExperimentOverrideServiceTestSuite) TestInvalidVariationInOverride() {
 		ID: "test_user_1",
 	}
 	// This override variation key does not exist in the experiment
-	s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = "invalid_variation_key"
+	s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, "invalid_variation_key")
 	decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext)
 	s.NoError(err)
 	s.Nil(decision.Variation)
 	s.Exactly(reasons.InvalidOverrideVariationAssignment, decision.Reason)
 }
 
+// Test concurrent use of the MapExperimentOverrideStore
+// Create 3 goroutines that set and get variations, and assert that all their sets take effect at the end
+func (s *ExperimentOverrideServiceTestSuite) TestMapExperimentOverridesStoreConcurrent() {
+	testDecisionContext := ExperimentDecisionContext{
+		Experiment:    &testExp1111,
+		ProjectConfig: s.mockConfig,
+	}
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, testExp1111Var2222.Key)
+		user1Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+			ID: "test_user_1",
+		})
+		s.NotNil(user1Decision.Variation)
+		s.Exactly(testExp1111Var2222.Key, user1Decision.Variation.Key)
+		wg.Done()
+	}()
+	wg.Add(1)
+	go func() {
+		s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}, testExp1111Var2222.Key)
+		user2Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+			ID: "test_user_2",
+		})
+		s.NotNil(user2Decision.Variation)
+		s.Exactly(testExp1111Var2222.Key, user2Decision.Variation.Key)
+		wg.Done()
+	}()
+	wg.Add(1)
+	go func() {
+		s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_3"}, testExp1111Var2222.Key)
+		user3Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+			ID: "test_user_3",
+		})
+		s.NotNil(user3Decision.Variation)
+		s.Exactly(testExp1111Var2222.Key, user3Decision.Variation.Key)
+		wg.Done()
+	}()
+	wg.Wait()
+	user1Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+		ID: "test_user_1",
+	})
+	user2Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+		ID: "test_user_2",
+	})
+	user3Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{
+		ID: "test_user_3",
+	})
+	s.NotNil(user1Decision.Variation)
+	s.Exactly(testExp1111Var2222.Key, user1Decision.Variation.Key)
+	s.NotNil(user2Decision.Variation)
+	s.Exactly(testExp1111Var2222.Key, user2Decision.Variation.Key)
+	s.NotNil(user3Decision.Variation)
+	s.Exactly(testExp1111Var2222.Key, user3Decision.Variation.Key)
+}
+
 func TestExperimentOverridesTestSuite(t *testing.T) {
 	suite.Run(t, new(ExperimentOverrideServiceTestSuite))
 }