Repository: Unleash/unleash-client-go
Pull request #57 (2019-10-19 21:35:00)

Don't send OnReady unless /client/features has returned successfully


Patch:
diff --git a/client.go b/client.go
index 39fc9b3..df5b043 100644
--- a/client.go
+++ b/client.go
@@ -304,8 +304,9 @@ func (uc Client) Warnings() <-chan error {
 	return uc.warnings
 }
 
-// Ready returns the ready signal for the client and is fired after the repository of locally stored
-// toggles has been loaded.
+// Ready returns the ready channel for the client. A value will be available on
+// the channel when the feature toggles have been loaded from the Unleash
+// server.
 func (uc Client) Ready() <-chan bool {
 	return uc.ready
 }
@@ -336,8 +337,9 @@ func (uc Client) getStrategy(name string) strategy.Strategy {
 	return nil
 }
 
-// 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.
+// WaitForReady will block until the client has loaded the feature toggles from
+// the Unleash server. It will return immediately if the toggles have already
+// been loaded,
 //
 // It is safe to call this method from multiple goroutines concurrently.
 func (uc *Client) WaitForReady() {
diff --git a/repository.go b/repository.go
index 59e8078..9780c58 100644
--- a/repository.go
+++ b/repository.go
@@ -3,7 +3,9 @@ package unleash
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
+	"net/url"
 	"sync"
 	"time"
 
@@ -13,12 +15,14 @@ import (
 type repository struct {
 	repositoryChannels
 	sync.RWMutex
-	options repositoryOptions
-	etag    string
-	close   chan struct{}
-	closed  chan struct{}
-	ctx     context.Context
-	cancel  func()
+	options       repositoryOptions
+	etag          string
+	close         chan struct{}
+	closed        chan struct{}
+	ctx           context.Context
+	cancel        func()
+	isReady       bool
+	refreshTicker *time.Ticker
 }
 
 func newRepository(options repositoryOptions, channels repositoryChannels) *repository {
@@ -27,6 +31,7 @@ func newRepository(options repositoryOptions, channels repositoryChannels) *repo
 		repositoryChannels: channels,
 		close:              make(chan struct{}),
 		closed:             make(chan struct{}),
+		refreshTicker:      time.NewTicker(options.refreshInterval),
 	}
 	ctx, cancel := context.WithCancel(context.Background())
 	repo.ctx = ctx
@@ -47,13 +52,22 @@ func newRepository(options repositoryOptions, channels repositoryChannels) *repo
 	return repo
 }
 
-func (r *repository) sync() {
-	r.fetch()
-	r.ready <- true
+func (r *repository) fetchAndReportError() {
+	err := r.fetch()
+	if err != nil {
+		if urlErr, ok := err.(*url.Error); !(ok && urlErr.Err == context.Canceled) {
+			r.err(err)
+		}
+	}
+	if !r.isReady && err == nil {
+		r.isReady = true
+		r.ready <- true
+	}
+}
 
+func (r *repository) sync() {
+	r.fetchAndReportError()
 	for {
-		refreshTimer := time.NewTimer(r.options.refreshInterval)
-
 		select {
 		case <-r.close:
 			if err := r.options.storage.Persist(); err != nil {
@@ -61,19 +75,18 @@ func (r *repository) sync() {
 			}
 			close(r.closed)
 			return
-		case <-refreshTimer.C:
-			r.fetch()
+		case <-r.refreshTicker.C:
+			r.fetchAndReportError()
 		}
 	}
 }
 
-func (r *repository) fetch() {
+func (r *repository) fetch() error {
 	u, _ := r.options.url.Parse("./client/features")
 
 	req, err := http.NewRequest("GET", u.String(), nil)
 	if err != nil {
-		r.err(err)
-		return
+		return err
 	}
 	req = req.WithContext(r.ctx)
 
@@ -91,27 +104,38 @@ func (r *repository) fetch() {
 
 	resp, err := r.options.httpClient.Do(req)
 	if err != nil {
-		r.err(err)
-		return
+		return err
 	}
 
 	defer resp.Body.Close()
 
 	if resp.StatusCode == http.StatusNotModified {
-		return
+		return nil
+	}
+	if err := statusIsOK(resp); err != nil {
+		return err
 	}
 
 	var featureResp api.FeatureResponse
 	dec := json.NewDecoder(resp.Body)
 	if err := dec.Decode(&featureResp); err != nil {
-		r.err(err)
-		return
+		return err
 	}
 
 	r.Lock()
 	r.etag = resp.Header.Get("Etag")
 	r.options.storage.Reset(featureResp.FeatureMap(), true)
 	r.Unlock()
+	return nil
+}
+
+func statusIsOK(resp *http.Response) error {
+	s := resp.StatusCode
+	if 200 <= s && s < 300 {
+		return nil
+	}
+
+	return fmt.Errorf("%s %s returned status code %d", resp.Request.Method, resp.Request.URL, s)
 }
 
 func (r *repository) getToggle(key string) *api.Feature {
@@ -130,5 +154,6 @@ func (r *repository) Close() error {
 	close(r.close)
 	r.cancel()
 	<-r.closed
+	r.refreshTicker.Stop()
 	return nil
 }
diff --git a/unleash.go b/unleash.go
index 7d2b15e..5565423 100644
--- a/unleash.go
+++ b/unleash.go
@@ -28,8 +28,8 @@ type MetricListener interface {
 // RepositoryListener defines an interface that can be implemented in order to receive events that are relevant to
 // the feature toggle repository.
 type RepositoryListener interface {
-	// OnReady is called once the repository has successfully loaded the options from the specified
-	// storage option.
+	// OnReady is called when the client has loaded the feature toggles from
+	// the Unleash server.
 	OnReady()
 }
 


Test patch:
diff --git a/metrics_test.go b/metrics_test.go
index 68345c3..5cdb945 100644
--- a/metrics_test.go
+++ b/metrics_test.go
@@ -127,13 +127,6 @@ func TestMetrics_DisabledMetrics(t *testing.T) {
 	assert.True(gock.IsDone(), "there should be no more mocks")
 }
 
-func writeJSON(rw http.ResponseWriter, x interface{}) {
-	enc := json.NewEncoder(rw)
-	if err := enc.Encode(x); err != nil {
-		panic(err)
-	}
-}
-
 // TestMetrics_SendMetricsFail tests that no metrics are lost if /client/metrics
 // fails temporarily.
 func TestMetrics_SendMetricsFail(t *testing.T) {
diff --git a/repository_test.go b/repository_test.go
new file mode 100644
index 0000000..ff94b62
--- /dev/null
+++ b/repository_test.go
@@ -0,0 +1,81 @@
+package unleash
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/Unleash/unleash-client-go/v3/internal/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+)
+
+// TestRepository_GetFeaturesFail tests that OnReady isn't fired unless
+// /client/features has returned successfully.
+func TestRepository_GetFeaturesFail(t *testing.T) {
+	assert := assert.New(t)
+	featuresCalls := make(chan int, 10)
+	var sendStatus200 int32
+	prevStatus := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		switch req.Method + " " + req.URL.Path {
+		case "POST /client/register":
+		case "GET /client/features":
+			status200 := atomic.LoadInt32(&sendStatus200) == 1
+			status := 0
+			if status200 {
+				status = 200
+				rw.WriteHeader(200)
+				writeJSON(rw, api.FeatureResponse{})
+			} else {
+				status = 400
+				rw.WriteHeader(400)
+			}
+			if status != prevStatus {
+				featuresCalls <- status
+				prevStatus = status
+			}
+		case "POST /client/metrics":
+		default:
+			t.Fatalf("Unexpected request: %+v", req)
+		}
+	}))
+	defer srv.Close()
+
+	ready := make(chan struct{})
+	mockListener := &MockedListener{}
+	mockListener.On("OnReady").Run(func(args mock.Arguments) { close(ready) }).Return()
+	mockListener.On("OnRegistered", mock.AnythingOfType("ClientData"))
+	mockListener.On("OnError", mock.MatchedBy(func(e error) bool {
+		return strings.HasSuffix(e.Error(), "/client/features returned status code 400")
+	})).Return()
+	mockListener.On("OnSent", mock.AnythingOfType("MetricsData")).Return()
+	client, err := NewClient(
+		WithUrl(srv.URL),
+		WithAppName(mockAppName),
+		WithInstanceId(mockInstanceId),
+		WithListener(mockListener),
+		WithRefreshInterval(time.Millisecond),
+	)
+	assert.Nil(err, "client should not return an error")
+
+	assert.Equal(400, <-featuresCalls)
+	select {
+	case <-ready:
+		t.Fatal("client is ready but it shouldn't be")
+	case <-time.NewTimer(time.Second).C:
+	}
+
+	atomic.StoreInt32(&sendStatus200, 1)
+	assert.Equal(200, <-featuresCalls)
+
+	select {
+	case <-ready:
+	case <-time.NewTimer(time.Second).C:
+		t.Fatal("client isn't ready but should be")
+	}
+	client.Close()
+}
diff --git a/unleash_test.go b/unleash_test.go
index 108eff7..76be4de 100644
--- a/unleash_test.go
+++ b/unleash_test.go
@@ -1,6 +1,9 @@
 package unleash
 
 import (
+	"encoding/json"
+	"net/http"
+
 	"github.com/stretchr/testify/mock"
 )
 
@@ -37,3 +40,10 @@ func (l *MockedListener) OnSent(payload MetricsData) {
 func (l *MockedListener) OnRegistered(payload ClientData) {
 	l.Called(payload)
 }
+
+func writeJSON(rw http.ResponseWriter, x interface{}) {
+	enc := json.NewEncoder(rw)
+	if err := enc.Encode(x); err != nil {
+		panic(err)
+	}
+}