fix: correctly handle exceptions >400 (#79)

Fixes #78 
Extracts status code from the returned error and acts on that. This limits more complex behavior, such as respecting the retry-after header.

changelog/@unreleased/pr-79.v2.yml

@@ -0,0 +1,7 @@
+type: fix
+fix:
+  description: "Fixes #78 \nExtracts status code from the returned error and acts
+    on that. This limits more complex behavior, such as respecting the retry-after
+    header."
+  links:
+  - https://github.com/palantir/conjure-go-runtime/pull/79

conjure-go-client/httpclient/client.go

@@ -97,14 +97,16 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp
 	failedURIs := map[string]struct{}{}
 	for i := 0; i < c.maxRetries; i++ {
 		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, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
-		} else if shouldThrottle, _ := internal.IsThrottleResponse(resp); shouldThrottle {
+		if retryOther, _ := internal.IsThrottleResponse(resp, err); retryOther {
 			// 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
+			// Immediately backoff and select the next URI.
+			// TODO(whickman): use the retry-after header once #81 is resolved
+			nextURI, offset = nextURIAndBackoff(nextURI, uris, offset, failedURIs, retrier)
+		} else if internal.IsUnavailableResponse(resp, err) {
+			// 503: go to next node
+			nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
+		} else 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, 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.
@@ -114,9 +116,6 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp
 			} else {
 				nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 			}
-		} else if internal.IsUnavailableResponse(resp) {
-			// 503: go to next node
-			nextURI, offset = nextURIOrBackoff(nextURI, uris, offset, failedURIs, retrier)
 		} else {
 			// The response was not a failure in any way, return the error
 			return resp, err
@@ -128,20 +127,33 @@ func (c *clientImpl) Do(ctx context.Context, params ...RequestParam) (*http.Resp
 	return resp, werror.Error("could not find live server")
 }
 
-// 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 and its offset immediately
+// If lastURI was already marked failed, we perform a backoff as determined by the retrier before returning the next URI and its offset.
+// 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{}{}
-	nextURIOffset = (offset + 1) % len(uris)
-	nextURI = uris[nextURIOffset]
+	nextURI, nextURIOffset = markFailedAndGetNextURI(failedURIs, lastURI, offset, uris)
 	// If the URI has failed before, perform a backoff
 	if performBackoff {
 		retrier.Next()
 	}
 	return nextURI, nextURIOffset
 }
 
+// Marks the current URI as failed, gets the next URI, and performs a backoff as determined by the retrier.
+func nextURIAndBackoff(lastURI string, uris []string, offset int, failedURIs map[string]struct{}, retrier retry.Retrier) (nextURI string, nextURIOffset int) {
+	nextURI, nextURIOffset = markFailedAndGetNextURI(failedURIs, lastURI, offset, uris)
+	retrier.Next()
+	return nextURI, nextURIOffset
+
+}
+
+func markFailedAndGetNextURI(failedURIs map[string]struct{}, lastURI string, offset int, uris []string) (string, int) {
+	failedURIs[lastURI] = struct{}{}
+	nextURIOffset := (offset + 1) % len(uris)
+	nextURI := uris[nextURIOffset]
+	return nextURI, nextURIOffset
+}
+
 func (c *clientImpl) doOnce(ctx context.Context, baseURI string, params ...RequestParam) (*http.Response, error) {
 
 	// 1. create the request

conjure-go-client/httpclient/error_decoder_test.go

@@ -23,6 +23,7 @@ import (
 	"testing"
 
 	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient"
+	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/internal"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -51,7 +52,7 @@ func TestErrorDecoder(t *testing.T) {
 		resp, err := client.Get(context.Background())
 		assert.EqualError(t, err, errPrefix+defaultDecoderMsg)
 		assert.Nil(t, resp)
-		gotStatusCode, ok := httpclient.StatusCodeFromError(err)
+		gotStatusCode, ok := internal.StatusCodeFromError(err)
 		assert.True(t, ok)
 		assert.Equal(t, statusCode, gotStatusCode)
 	})

conjure-go-client/httpclient/failover_test.go

@@ -20,6 +20,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/internal"
 	"github.com/palantir/pkg/httpserver"
@@ -50,6 +51,41 @@ func TestFailover503(t *testing.T) {
 	assert.Equal(t, 3, n)
 }
 
+func TestFailover429(t *testing.T) {
+	n := 0
+	handler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		n++
+		if n == 3 {
+			rw.WriteHeader(http.StatusOK)
+			_, err := rw.Write([]byte("body"))
+			require.NoError(t, err)
+		} else {
+			rw.WriteHeader(http.StatusTooManyRequests)
+			_, err := rw.Write([]byte("body"))
+			require.NoError(t, err)
+		}
+	})
+
+	s1 := httptest.NewServer(handler)
+	s2 := httptest.NewServer(handler)
+	s3 := httptest.NewServer(handler)
+
+	backoff := 5 * time.Millisecond
+	cli, err := NewClient(WithBaseURLs([]string{s1.URL, s2.URL, s3.URL}), WithInitialBackoff(backoff), WithMaxBackoff(backoff))
+	require.NoError(t, err)
+
+	timer := time.NewTimer(backoff)
+	_, err = cli.Do(context.Background(), WithRequestMethod("GET"))
+	select {
+	case <-timer.C:
+		break
+	default:
+		t.Error("Timer was not complete, back-off did not appear to occur")
+	}
+	assert.Nil(t, err)
+	assert.Equal(t, 3, n)
+}
+
 func TestFailover200(t *testing.T) {
 	n := 0
 	handler := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {

conjure-go-client/httpclient/internal/errors.go

@@ -0,0 +1,34 @@
+// Copyright (c) 2020 Palantir Technologies. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package internal
+
+import (
+	werror "github.com/palantir/witchcraft-go-error"
+)
+
+// StatusCodeFromError retrieves the 'statusCode' parameter from the provided werror.
+// If the error is not a werror or does not have the statusCode param, ok is false.
+//
+// The default client error decoder sets the statusCode parameter on its returned errors. Note that, if a custom error
+// decoder is used, this function will only return a status code for the error if the custom decoder sets a 'statusCode'
+// parameter on the error.
+func StatusCodeFromError(err error) (statusCode int, ok bool) {
+	statusCodeI, ok := werror.ParamFromError(err, "statusCode")
+	if !ok {
+		return 0, false
+	}
+	statusCode, ok = statusCodeI.(int)
+	return statusCode, ok
+}

conjure-go-client/httpclient/internal/retry.go

@@ -73,7 +73,11 @@ func IsRetryOtherResponse(resp *http.Response) (bool, *url.URL) {
 	return true, locationURL
 }
 
-func IsThrottleResponse(resp *http.Response) (bool, time.Duration) {
+func IsThrottleResponse(resp *http.Response, err error) (bool, time.Duration) {
+	errCode, ok := StatusCodeFromError(err)
+	if ok && errCode == StatusCodeThrottle {
+		return true, 0
+	}
 	if resp == nil || resp.StatusCode != StatusCodeThrottle {
 		return false, 0
 	}
@@ -93,7 +97,11 @@ func IsThrottleResponse(resp *http.Response) (bool, time.Duration) {
 	return true, time.Until(retryAfterDate)
 }
 
-func IsUnavailableResponse(resp *http.Response) bool {
+func IsUnavailableResponse(resp *http.Response, err error) bool {
+	errCode, ok := StatusCodeFromError(err)
+	if ok && errCode == StatusCodeUnavailable {
+		return true
+	}
 	if resp == nil || resp.StatusCode != StatusCodeUnavailable {
 		return false
 	}

conjure-go-client/httpclient/internal/retry_test.go

@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/internal"
+	werror "github.com/palantir/witchcraft-go-error"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -66,6 +67,18 @@ func TestRetryResponseParsers(t *testing.T) {
 			},
 			IsThrottle: true,
 		},
+		{
+			Name:       "429 throttle in error",
+			Response:   nil,
+			RespErr:    werror.Error("error", werror.SafeParam("statusCode", 429)),
+			IsThrottle: true,
+		},
+		{
+			Name:          "503 unavailable in error",
+			Response:      nil,
+			RespErr:       werror.Error("error", werror.SafeParam("statusCode", 503)),
+			IsUnavailable: true,
+		},
 		{
 			Name: "429 throttle with Retry-After seconds",
 			Response: &http.Response{
@@ -93,12 +106,12 @@ func TestRetryResponseParsers(t *testing.T) {
 				}
 			}
 
-			isThrottle, throttleDur := internal.IsThrottleResponse(test.Response)
+			isThrottle, throttleDur := internal.IsThrottleResponse(test.Response, test.RespErr)
 			if assert.Equal(t, test.IsThrottle, isThrottle) {
 				assert.WithinDuration(t, time.Now().Add(test.ThrottleDuration), time.Now().Add(throttleDur), time.Second)
 			}
 
-			isUnavailable := internal.IsUnavailableResponse(test.Response)
+			isUnavailable := internal.IsUnavailableResponse(test.Response, test.RespErr)
 			assert.Equal(t, test.IsUnavailable, isUnavailable)
 		})
 	}

conjure-go-client/httpclient/response_error_decoder_middleware.go

@@ -69,17 +69,7 @@ func (d restErrorDecoder) DecodeError(resp *http.Response) error {
 	return werror.Error("server returned a status >= 400", werror.SafeParam("statusCode", resp.StatusCode))
 }
 
-// StatusCodeFromError retrieves the 'statusCode' parameter from the provided werror.
-// If the error is not a werror or does not have the statusCode param, ok is false.
-//
-// The default client error decoder sets the statusCode parameter on its returned errors. Note that, if a custom error
-// decoder is used, this function will only return a status code for the error if the custom decoder sets a 'statusCode'
-// parameter on the error.
+// StatusCodeFromError wraps the internal StatusCodeFromError func. For behavior details, see its docs.
 func StatusCodeFromError(err error) (statusCode int, ok bool) {
-	statusCodeI, ok := werror.ParamFromError(err, "statusCode")
-	if !ok {
-		return 0, false
-	}
-	statusCode, ok = statusCodeI.(int)
-	return statusCode, ok
+	return internal.StatusCodeFromError(err)
 }

conjure-go-client/httpclient/response_error_decoder_middleware_test.go

@@ -23,6 +23,7 @@ import (
 	"testing"
 
 	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient"
+	"github.com/palantir/conjure-go-runtime/conjure-go-client/httpclient/internal"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -38,7 +39,7 @@ func TestErrorsMiddleware(t *testing.T) {
 
 		_, err = client.Do(ctx, httpclient.WithRequestMethod(http.MethodGet))
 		require.Error(t, err)
-		status, ok := httpclient.StatusCodeFromError(err)
+		status, ok := internal.StatusCodeFromError(err)
 		require.True(t, ok)
 		require.Equal(t, http.StatusNotFound, status)
 	})