Retry only when HTTP request fails

`PerformRequest` retried even on valid responses, e.g. when ES
returned a 404. However, 404 can be a valid response from Elasticsearch
for certain services, e.g. the Exists service. Now we only
retry when `HttpRequest.Do` returns an error.

This commit also refactors error handling for services that return a
404 as normal behavior, e.g. `ExistsService`, `IndicesExistsService`
and `IndicesExistsTypeService`. The error code is now handled in the
service, not in the generic error handler (`errors.go`). Instead, one
can pass HTTP error codes to ignore now to the error handler and to
`PerformRequest`.

Author: olivere

client.go

@@ -22,7 +22,7 @@ import (
 
 const (
 	// Version is the current version of Elastic.
-	Version = "3.0.3"
+	Version = "3.0.4"
 
 	// DefaultUrl is the default endpoint of Elasticsearch on the local machine.
 	// It is used e.g. when initializing a new Client without a specific URL.
@@ -900,7 +900,11 @@ func (c *Client) mustActiveConn() error {
 
 // PerformRequest does a HTTP request to Elasticsearch.
 // It returns a response and an error on failure.
-func (c *Client) PerformRequest(method, path string, params url.Values, body interface{}) (*Response, error) {
+//
+// Optionally, a list of HTTP error codes to ignore can be passed.
+// This is necessary for services that expect e.g. HTTP status 404 as a
+// valid outcome (Exists, IndicesExists, IndicesTypeExists).
+func (c *Client) PerformRequest(method, path string, params url.Values, body interface{}, ignoreErrors ...int) (*Response, error) {
 	start := time.Now().UTC()
 
 	c.mu.RLock()
@@ -992,15 +996,9 @@ func (c *Client) PerformRequest(method, path string, params url.Values, body int
 		}
 
 		// Check for errors
-		if err := checkResponse((*http.Request)(req), res); err != nil {
-			retries -= 1
-			if retries <= 0 {
-				return nil, err
-			}
-			retried = true
-			time.Sleep(time.Duration(retryWaitMsec) * time.Millisecond)
-			retryWaitMsec += retryWaitMsec
-			continue // try again
+		if err := checkResponse((*http.Request)(req), res, ignoreErrors...); err != nil {
+			// No retry if request succeeded
+			return nil, err
 		}
 
 		// Tracing

client_test.go

@@ -7,6 +7,7 @@ package elastic
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"log"
 	"net/http"
 	"regexp"
@@ -664,11 +665,12 @@ func (tr *failingTransport) RoundTrip(r *http.Request) (*http.Response, error) {
 	return http.DefaultTransport.RoundTrip(r)
 }
 
-func TestPerformRequestWithMaxRetries(t *testing.T) {
+func TestPerformRequestRetryOnHttpError(t *testing.T) {
 	var numFailedReqs int
 	fail := func(r *http.Request) (*http.Response, error) {
 		numFailedReqs += 1
-		return &http.Response{Request: r, StatusCode: 400}, nil
+		//return &http.Response{Request: r, StatusCode: 400}, nil
+		return nil, errors.New("request failed")
 	}
 
 	// Run against a failing endpoint and see if PerformRequest
@@ -693,3 +695,33 @@ func TestPerformRequestWithMaxRetries(t *testing.T) {
 		t.Errorf("expected %d failed requests; got: %d", 5, numFailedReqs)
 	}
 }
+
+func TestPerformRequestNoRetryOnValidButUnsuccessfulHttpStatus(t *testing.T) {
+	var numFailedReqs int
+	fail := func(r *http.Request) (*http.Response, error) {
+		numFailedReqs += 1
+		return &http.Response{Request: r, StatusCode: 500}, nil
+	}
+
+	// Run against a failing endpoint and see if PerformRequest
+	// retries correctly.
+	tr := &failingTransport{path: "/fail", fail: fail}
+	httpClient := &http.Client{Transport: tr}
+
+	client, err := NewClient(SetHttpClient(httpClient), SetMaxRetries(5))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	res, err := client.PerformRequest("GET", "/fail", nil, nil)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+	if res != nil {
+		t.Fatal("expected no response")
+	}
+	// Retry should not have triggered additional requests because
+	if numFailedReqs != 1 {
+		t.Errorf("expected %d failed requests; got: %d", 1, numFailedReqs)
+	}
+}

errors.go

@@ -21,29 +21,38 @@ var (
 // an error returned from Elasticsearch.
 //
 // HTTP status codes between in the range [200..299] are considered successful.
-// Also, HTTP requests with method HEAD that return 404 are considered
-// to be no errors. All other request/response combinations return an error.
+// All other errors are considered errors except they are specified in
+// ignoreErrors. This is necessary because for some services, HTTP status 404
+// is a valid response from Elasticsearch (e.g. the Exists service).
 //
 // The func tries to parse error details as returned from Elasticsearch
 // and encapsulates them in type elastic.Error.
-func checkResponse(req *http.Request, res *http.Response) error {
+func checkResponse(req *http.Request, res *http.Response, ignoreErrors ...int) error {
 	// 200-299 are valid status codes
 	if res.StatusCode >= 200 && res.StatusCode <= 299 {
 		return nil
 	}
-	// HEAD requests with 404 are not an error
-	if req.Method == "HEAD" && res.StatusCode == 404 {
-		return nil
+	// Ignore certain errors?
+	for _, code := range ignoreErrors {
+		if code == res.StatusCode {
+			return nil
+		}
 	}
+	return createResponseError(res)
+}
+
+// createResponseError creates an Error structure from the HTTP response,
+// its status code and the error information sent by Elasticsearch.
+func createResponseError(res *http.Response) error {
 	if res.Body == nil {
 		return &Error{Status: res.StatusCode}
 	}
-	slurp, err := ioutil.ReadAll(res.Body)
+	data, err := ioutil.ReadAll(res.Body)
 	if err != nil {
 		return &Error{Status: res.StatusCode}
 	}
 	errReply := new(Error)
-	err = json.Unmarshal(slurp, errReply)
+	err = json.Unmarshal(data, errReply)
 	if err != nil {
 		return &Error{Status: res.StatusCode}
 	}

errors_test.go

@@ -104,7 +104,7 @@ func TestResponseErrorHTML(t *testing.T) {
 	}
 }
 
-func TestResponseErrorWithHeadMethodAndNotFound(t *testing.T) {
+func TestResponseErrorWithIgnore(t *testing.T) {
 	raw := "HTTP/1.1 404 Not Found\r\n" +
 		"\r\n" +
 		`{"some":"response"}` + "\r\n"
@@ -120,6 +120,10 @@ func TestResponseErrorWithHeadMethodAndNotFound(t *testing.T) {
 		t.Fatal(err)
 	}
 	err = checkResponse(req, resp)
+	if err == nil {
+		t.Fatalf("expected error; got: %v", err)
+	}
+	err = checkResponse(req, resp, 404) // ignore 404 errors
 	if err != nil {
 		t.Fatalf("expected no error; got: %v", err)
 	}

exists.go

@@ -158,7 +158,7 @@ func (s *ExistsService) Do() (bool, error) {
 	}
 
 	// Get HTTP response
-	res, err := s.client.PerformRequest("HEAD", path, params, nil)
+	res, err := s.client.PerformRequest("HEAD", path, params, nil, 404)
 	if err != nil {
 		return false, err
 	}

indices_exists.go

@@ -6,6 +6,7 @@ package elastic
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"strings"
 
@@ -131,16 +132,18 @@ func (s *IndicesExistsService) Do() (bool, error) {
 	}
 
 	// Get HTTP response
-	res, err := s.client.PerformRequest("HEAD", path, params, nil)
+	res, err := s.client.PerformRequest("HEAD", path, params, nil, 404)
 	if err != nil {
 		return false, err
 	}
 
 	// Return operation response
-	if res.StatusCode == 200 {
+	switch res.StatusCode {
+	case http.StatusOK:
 		return true, nil
-	} else if res.StatusCode == 404 {
+	case http.StatusNotFound:
 		return false, nil
+	default:
+		return false, fmt.Errorf("elastic: got HTTP code %d when it should have been either 200 or 404", res.StatusCode)
 	}
-	return false, fmt.Errorf("elastic: got HTTP code %d when it should have been either 200 or 404", res.StatusCode)
 }

indices_exists_type.go

@@ -6,6 +6,7 @@ package elastic
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"strings"
 
@@ -143,16 +144,18 @@ func (s *IndicesExistsTypeService) Do() (bool, error) {
 	}
 
 	// Get HTTP response
-	res, err := s.client.PerformRequest("HEAD", path, params, nil)
+	res, err := s.client.PerformRequest("HEAD", path, params, nil, 404)
 	if err != nil {
 		return false, err
 	}
 
 	// Return operation response
-	if res.StatusCode == 200 {
+	switch res.StatusCode {
+	case http.StatusOK:
 		return true, nil
-	} else if res.StatusCode == 404 {
+	case http.StatusNotFound:
 		return false, nil
+	default:
+		return false, fmt.Errorf("elastic: got HTTP code %d when it should have been either 200 or 404", res.StatusCode)
 	}
-	return false, fmt.Errorf("elastic: got HTTP code %d when it should have been either 200 or 404", res.StatusCode)
 }