Close response

httpclient/client.go

@@ -1,14 +1,12 @@
 package httpclient
 
 import (
-	"fmt"
+	"bytes"
 	"io"
+	"io/ioutil"
 	"net/http"
 	"time"
 
-	"bytes"
-	"io/ioutil"
-
 	"github.com/gojektech/heimdall"
 	"github.com/gojektech/valkyrie"
 	"github.com/pkg/errors"
@@ -135,6 +133,10 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 	var response *http.Response
 
 	for i := 0; i <= c.retryCount; i++ {
+		if response != nil {
+			response.Body.Close()
+		}
+
 		var err error
 		response, err = c.client.Do(request)
 		if bodyReader != nil {
@@ -152,8 +154,6 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 		}
 
 		if response.StatusCode >= http.StatusInternalServerError {
-			multiErr.Push(fmt.Sprintf("server error: %d", response.StatusCode))
-
 			backoffTime := c.retrier.NextInterval(i)
 			time.Sleep(backoffTime)
 			continue

httpclient/client_test.go

@@ -223,7 +223,7 @@ func TestHTTPClientGetRetriesOnFailure(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Get(server.URL, http.Header{})
-	require.Error(t, err, "should have failed to make GET request")
+	require.NoError(t, err, "should have failed to make GET request")
 
 	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
 	require.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
@@ -278,7 +278,7 @@ func TestHTTPClientPostRetriesOnFailure(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Post(server.URL, strings.NewReader("a=1"), http.Header{})
-	require.Error(t, err, "should have failed to make GET request")
+	require.NoError(t, err, "should have failed to make GET request")
 
 	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
 	require.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
@@ -310,7 +310,7 @@ func BenchmarkHTTPClientPostRetriesOnFailure(b *testing.B) {
 	}
 }
 
-func TestHTTPClientGetReturnsAllErrorsIfRetriesFail(t *testing.T) {
+func TestHTTPClientGetReturnsNoErrorsIfRetriesFailWith5xx(t *testing.T) {
 	count := 0
 	noOfRetries := 2
 	backoffInterval := 1 * time.Millisecond
@@ -332,13 +332,11 @@ func TestHTTPClientGetReturnsAllErrorsIfRetriesFail(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Get(server.URL, http.Header{})
-	require.Error(t, err, "should have failed to make GET request")
+	require.NoError(t, err)
 
 	require.Equal(t, noOfRetries+1, count)
 	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
 	require.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
-
-	assert.Equal(t, "server error: 500, server error: 500, server error: 500", err.Error())
 }
 
 func TestHTTPClientGetReturnsNoErrorsIfRetrySucceeds(t *testing.T) {
@@ -393,7 +391,7 @@ func TestHTTPClientGetReturnsErrorOnClientCallFailure(t *testing.T) {
 	assert.Equal(t, "Get : unsupported protocol scheme \"\"", err.Error())
 }
 
-func TestHTTPClientGetReturnsErrorOn5xxFailure(t *testing.T) {
+func TestHTTPClientGetReturnsNoErrorOn5xxFailure(t *testing.T) {
 	client := NewClient(WithHTTPTimeout(10 * time.Millisecond))
 
 	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
@@ -405,11 +403,18 @@ func TestHTTPClientGetReturnsErrorOn5xxFailure(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Get(server.URL, http.Header{})
-	require.Error(t, err, "should have failed to make GET request")
-
+	require.NoError(t, err)
 	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
 
-	assert.Equal(t, "server error: 500", err.Error())
+}
+
+func TestHTTPClientGetReturnsErrorOnFailure(t *testing.T) {
+	client := NewClient(WithHTTPTimeout(10 * time.Millisecond))
+
+	response, err := client.Get("url_doenst_exist", http.Header{})
+	require.EqualError(t, err, "Get url_doenst_exist: unsupported protocol scheme \"\"")
+	require.Nil(t, response)
+
 }
 
 type myHTTPClient struct {

hystrix/hystrix_client.go

@@ -2,7 +2,6 @@ package hystrix
 
 import (
 	"bytes"
-	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -42,6 +41,7 @@ const (
 )
 
 var _ heimdall.Client = (*Client)(nil)
+var err5xx = errors.New("server returned 5xx status code")
 
 // NewClient returns a new instance of hystrix Client
 func NewClient(opts ...Option) *Client {
@@ -161,6 +161,10 @@ func (hhc *Client) Do(request *http.Request) (*http.Response, error) {
 	}
 
 	for i := 0; i <= hhc.retryCount; i++ {
+		if response != nil {
+			response.Body.Close()
+		}
+
 		err = hystrix.Do(hhc.hystrixCommandName, func() error {
 			response, err = hhc.client.Do(request)
 			if bodyReader != nil {
@@ -174,7 +178,7 @@ func (hhc *Client) Do(request *http.Request) (*http.Response, error) {
 			}
 
 			if response.StatusCode >= http.StatusInternalServerError {
-				return fmt.Errorf("Server is down: returned status code: %d", response.StatusCode)
+				return err5xx
 			}
 			return nil
 		}, hhc.fallbackFunc)
@@ -188,5 +192,9 @@ func (hhc *Client) Do(request *http.Request) (*http.Response, error) {
 		break
 	}
 
+	if err == err5xx {
+		return response, nil
+	}
+
 	return response, err
 }

hystrix/hystrix_client_test.go

@@ -259,7 +259,6 @@ func TestHystrixHTTPClientPatchSuccess(t *testing.T) {
 }
 
 func TestHystrixHTTPClientRetriesGetOnFailure(t *testing.T) {
-	count := 0
 	backoffInterval := 1 * time.Millisecond
 	maximumJitterInterval := 1 * time.Millisecond
 
@@ -275,6 +274,29 @@ func TestHystrixHTTPClientRetriesGetOnFailure(t *testing.T) {
 		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
 	)
 
+	response, err := client.Get("url_doesnt_exist", http.Header{})
+	require.EqualError(t, err, `Get url_doesnt_exist: unsupported protocol scheme ""`)
+
+	assert.Nil(t, response)
+}
+
+func TestHystrixHTTPClientRetriesGetOnFailure5xx(t *testing.T) {
+	count := 0
+	backoffInterval := 1 * time.Millisecond
+	maximumJitterInterval := 1 * time.Millisecond
+
+	client := NewClient(
+		WithHTTPTimeout(10*time.Millisecond),
+		WithCommandName("some_command_name_5xx"),
+		WithHystrixTimeout(10),
+		WithMaxConcurrentRequests(100),
+		WithErrorPercentThreshold(10),
+		WithSleepWindow(100),
+		WithRequestVolumeThreshold(10),
+		WithRetryCount(3),
+		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
+	)
+
 	dummyHandler := func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusInternalServerError)
 		_, _ = w.Write([]byte(`{ "response": "something went wrong" }`))
@@ -285,7 +307,7 @@ func TestHystrixHTTPClientRetriesGetOnFailure(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Get(server.URL, http.Header{})
-	require.Error(t, err)
+	require.NoError(t, err)
 
 	assert.Equal(t, 4, count)
 
@@ -349,10 +371,9 @@ func TestHystrixHTTPClientRetriesPostOnFailure(t *testing.T) {
 	defer server.Close()
 
 	response, err := client.Post(server.URL, strings.NewReader("a=1&b=2"), http.Header{})
-	require.Error(t, err)
+	require.NoError(t, err)
 
 	assert.Equal(t, 4, count)
-
 	assert.Equal(t, http.StatusInternalServerError, response.StatusCode)
 	assert.JSONEq(t, `{ "response": "something went wrong" }`, respBody(t, response))
 }