Fix httpclient round robin behavior (#16)

conjure-go-client/httpclient/client.go

@@ -94,24 +94,24 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp
 		resp, err = c.doOnce(ctx, nextURI, params...)
 		if resp == nil {
 			// If we get a nil response, we can assume there is a problem with host and can move on to the next.
-			nextURI = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
+			nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 		} else if shouldThrottle, _ := internal.IsThrottleResponse(resp); shouldThrottle {
 			// 429: throttle
 			// Ideally we should avoid hitting this URI until it's next available. In the interest of avoiding
 			// complex state in the client that will be replaced with a service-mesh, we will simply move on to the next
 			// available URI
-			nextURI = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
+			nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 		} else if shouldTryOther, otherURI := internal.IsRetryOtherResponse(resp); shouldTryOther {
 			// 308: go to next node, or particular node if provided.
 			if otherURI != nil {
 				nextURI = otherURI.String()
 				retrier.Reset()
 			} else {
-				nextURI = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
+				nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 			}
 		} else if internal.IsUnavailableResponse(resp) {
 			// 503: go to next node
-			nextURI = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
+			nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 		} else {
 			// The response was not a failure in any way, return the error
 			return resp, err
@@ -124,16 +124,17 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp
 }
 
 // If lastURI was already marked failed, we perform a backoff as determined by the retrier.
-// Otherwise, we add lastURI to failedURIs and return the next URI immediately.
-func nextURIOrBackoff(lastURI string, uris []string, offset int, failedURIs map[string]struct{}, retrier retry.Retrier) string {
+// Otherwise, we add lastURI to failedURIs and return the next URI and its offset immediately
+func nextURIOrBackoff(lastURI string, uris []string, offset int, failedURIs map[string]struct{}, retrier retry.Retrier) (nextURI string, nextURIOffset int) {
 	_, performBackoff := failedURIs[lastURI]
 	failedURIs[lastURI] = struct{}{}
-	nextURI := uris[(offset+1)%len(uris)]
+	nextURIOffset = (offset + 1) % len(uris)
+	nextURI = uris[nextURIOffset]
 	// If the URI has failed before, perform a backoff
 	if performBackoff {
 		retrier.Next()
 	}
-	return nextURI
+	return nextURI, nextURIOffset
 }
 
 func (c *clientImpl) doOnce(ctx context.Context, baseURI string, params ...RequestParam) (*http.Response, error) {

conjure-go-client/httpclient/failover_test.go

@@ -156,3 +156,21 @@ func TestSleep(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, 2, n)
 }
+
+func TestRoundRobin(t *testing.T) {
+	requestsPerSever := make([]int, 3)
+	getHandler := func(i int) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			requestsPerSever[i]++
+			rw.WriteHeader(http.StatusServiceUnavailable)
+		})
+	}
+	s0 := httptest.NewServer(getHandler(0))
+	s1 := httptest.NewServer(getHandler(1))
+	s2 := httptest.NewServer(getHandler(2))
+	cli, err := NewClient(WithBaseURLs([]string{s0.URL, s1.URL, s2.URL}))
+	require.NoError(t, err)
+	_, err = cli.Do(context.Background(), WithRequestMethod("GET"))
+	assert.Error(t, err)
+	assert.Equal(t, []int{2, 2, 2}, requestsPerSever)
+}