Repository: Unleash/unleash-client-go
Pull request #58 (2019-10-20 19:27:48)

Detect data races in Client.IsEnabled in tests
This PR makes `go test -race ./...` fail with
```
--- FAIL: TestClientSpecificationSuite (0.12s)
    --- FAIL: TestClientSpecificationSuite/TestClientSpecification (0.11s)
        --- FAIL: TestClientSpecificationSuite/TestClientSpecification/05-gradual-rollout-random-strategy (0.01s)
            --- FAIL: TestClientSpecificationSuite/TestClientSpecification/05-gradual-rollout-random-strategy/Feature.A5_should_be_enabled (0.00s)
                testing.go:809: race detected during execution of test
            testing.go:809: race detected during execution of test
        testing.go:809: race detected during execution of test
    testing.go:809: race detected during execution of test
FAIL
```
The detected data race is fixed in https://github.com/Unleash/unleash-client-go/pull/56 and by adding the relevant parts of that PR on top of this one `go test -race ./...` pass.

This PR also documents that Client.IsEnabled and Client.WaitForReady are safe to use from multiple goroutines concurrently.

Obviously, this PR should not be merged before https://github.com/Unleash/unleash-client-go/pull/56 has been merged.


Patch:
diff --git a/client.go b/client.go
index c490a57..b5bb3ec 100644
--- a/client.go
+++ b/client.go
@@ -222,7 +222,9 @@ func (uc *Client) sync() {
 	}
 }
 
-// IsEnabled queries whether or not the specified feature is enabled or not.
+// IsEnabled queries whether the specified feature is enabled or not.
+//
+// It is safe to call this method from multiple goroutines concurrently.
 func (uc Client) IsEnabled(feature string, options ...FeatureOption) (enabled bool) {
 	defer func() {
 		uc.metrics.count(feature, enabled)
@@ -319,6 +321,8 @@ func (uc Client) getStrategy(name string) strategy.Strategy {
 
 // WaitForReady will block until the internal repository has loaded the feature toggles from the
 // storage engine. It will return immediately if the repository is already ready.
+//
+// It is safe to call this method from multiple goroutines concurrently.
 func (uc *Client) WaitForReady() {
 	<-uc.onReady
 }


Test patch:
diff --git a/spec_test.go b/spec_test.go
index 7e9ac4d..a7362af 100644
--- a/spec_test.go
+++ b/spec_test.go
@@ -2,16 +2,18 @@ package unleash
 
 import (
 	"encoding/json"
+	"os"
+	"path/filepath"
+	"sync"
+	"testing"
+
 	"github.com/Unleash/unleash-client-go/v3/context"
 	"github.com/Unleash/unleash-client-go/v3/internal/api"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
 	"gopkg.in/h2non/gock.v1"
-	"os"
-	"path/filepath"
-	"testing"
-	)
+)
 
 const mockHost = "http://unleash-apu"
 const specFolder = "./testdata/client-specification/specifications"
@@ -33,14 +35,23 @@ type TestCase struct {
 func (tc TestCase) RunWithClient(client *Client) func(*testing.T) {
 	return func(t *testing.T) {
 		client.WaitForReady()
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			// Call IsEnabled concurrently with itself to catch
+			// potential data races with go test -race.
+			client.IsEnabled(tc.ToggleName, WithContext(tc.Context))
+			wg.Done()
+		}()
 		result := client.IsEnabled(tc.ToggleName, WithContext(tc.Context))
+		wg.Wait()
 		assert.Equal(t, tc.ExpectedResult, result)
 	}
 }
 
 type TestDefinition struct {
-	Name  string    `json:"name"`
-	State TestState `json:"state"`
+	Name  string     `json:"name"`
+	State TestState  `json:"state"`
 	Tests []TestCase `json:"tests"`
 }
 
@@ -66,7 +77,7 @@ func (td TestDefinition) Mock(listener interface{}) (*Client, error) {
 	)
 }
 
-func (td TestDefinition) Unmock() () {
+func (td TestDefinition) Unmock() {
 	gock.OffAll()
 }