common/lhttp: close the Response.Body on non-200s

common/lhttp/client_test.go

Index: common/lhttp/client_test.go
diff --git a/common/lhttp/client_test.go b/common/lhttp/client_test.go
index a6c721d51aeee95b6532bce7736038b55238d000..26d2f51324e968af92acceedbfdd649816b746fa 100644
--- a/common/lhttp/client_test.go
+++ b/common/lhttp/client_test.go
@@ -12,11 +12,11 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"sync"
 	"testing"
 
 	"golang.org/x/net/context"
 
-	"github.com/luci/luci-go/common/errors"
 	"github.com/luci/luci-go/common/retry"
 
 	"github.com/maruel/ut"
@@ -125,8 +125,7 @@ func TestNewRequestGETFail(t *testing.T) {
 	})
 
 	status, err := clientReq()
-	ut.AssertEqual(t, true, errors.IsTransient(err))

[mcgreevy, 2016/12/12 23:57:39]

Why are you removing these IsTransient checks?

[djd-OOO-Apr2017, 2016/12/13 00:18:09]

I've changed Retry so that it now wraps the final error to add the "attempts"
suffix. AFAICT, the fact that the last error was transient is not important for
the caller of Retry so I was okay removing it.

-	ut.AssertEqual(t, "http request failed: Internal Server Error (HTTP 500)", err.Error())
+	ut.AssertEqual(t, "http request failed: Internal Server Error (HTTP 500) (attempts: 4)", err.Error())
 	ut.AssertEqual(t, 500, status)
 }
 
@@ -169,8 +168,7 @@ func TestGetJSONBadResult(t *testing.T) {
 
 	actual := map[string]string{}
 	status, err := GetJSON(ctx, fast, http.DefaultClient, ts.URL, &actual)
-	ut.AssertEqual(t, true, errors.IsTransient(err))
-	ut.AssertEqual(t, "bad response "+ts.URL+": invalid character 'y' looking for beginning of value", err.Error())
+	ut.AssertEqual(t, "bad response "+ts.URL+": invalid character 'y' looking for beginning of value (attempts: 4)", err.Error())
 	ut.AssertEqual(t, 200, status)
 	ut.AssertEqual(t, map[string]string{}, actual)
 }
@@ -188,8 +186,7 @@ func TestGetJSONBadResultIgnore(t *testing.T) {
 	defer ts.Close()
 
 	status, err := GetJSON(ctx, fast, http.DefaultClient, ts.URL, nil)
-	ut.AssertEqual(t, true, errors.IsTransient(err))
-	ut.AssertEqual(t, "bad response "+ts.URL+": invalid character 'y' looking for beginning of value", err.Error())
+	ut.AssertEqual(t, "bad response "+ts.URL+": invalid character 'y' looking for beginning of value (attempts: 4)", err.Error())
 	ut.AssertEqual(t, 200, status)
 }
 
@@ -203,7 +200,7 @@ func TestGetJSONBadContentTypeIgnore(t *testing.T) {
 	defer ts.Close()
 
 	status, err := GetJSON(ctx, fast, http.DefaultClient, ts.URL, nil)
-	ut.AssertEqual(t, "unexpected Content-Type, expected \"application/json\", got \"text/plain; charset=utf-8\"", err.Error())
+	ut.AssertEqual(t, "unexpected Content-Type, expected \"application/json\", got \"text/plain; charset=utf-8\" (attempts: 4)", err.Error())
 	ut.AssertEqual(t, 200, status)
 }
 
@@ -253,6 +250,95 @@ func TestPostJSONwithHeaders(t *testing.T) {
 	ut.AssertEqual(t, 1, serverCalls)
 }
 
+func TestNewRequestClosesBody(t *testing.T) {
+	ctx := context.Background()
+	serverCalls := 0
+
+	// Return a 500 for the first 2 requests.
+	ts := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			defer r.Body.Close()
+			serverCalls++
+			if serverCalls <= 2 {
+				w.WriteHeader(500)
+			}
+			fmt.Fprintf(w, "Hello World!\n")
+		}))
+	defer ts.Close()
+
+	rt := &trackingRoundTripper{RoundTripper: http.DefaultTransport}
+	hc := &http.Client{Transport: rt}
+	httpReq := httpReqGen("GET", ts.URL, nil)
+
+	clientCalls := 0
+	var lastResp *http.Response
+	req := NewRequest(ctx, hc, fast, httpReq, func(resp *http.Response) error {
+		clientCalls++
+		lastResp = resp
+		return resp.Body.Close()
+	})
+
+	status, err := req()
+	if err != nil {
+		t.Fatalf("req returned err %v, want nil", err)
+	}
+	if got, want := status, http.StatusOK; got != want {

[M-A Ruel, 2016/12/12 17:16:39]

That's an unusual style (to me) but it's fine.

In general I use the words "actual" and "expected", although they are longer to
type and bytes are pricey.

[djd-OOO-Apr2017, 2016/12/13 00:18:09]

I agree. I prefer the work "actual" much more than "got" (I have a bit of a
personal vendetta against that awful word). However the "got/want" is the
prevailing Go style and used consistently throughout the community:
https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures
(I can find even better resources internally if you're interested).

If you prefer, I can migrate the rest of the tests to be consistent? Let me know
what you think.

[M-A Ruel, 2016/12/13 01:55:59]

That's totally bikeshedding level at that point, I really don't care. :) Let's
move on and be productive.

That said, I love that the immediate sentence after the code snippet says:
"Note that the order here is actual != expected, and the message uses that order
too"

as in: "The words we meant are 'actual' and 'expected' but we use something else
just to mess with you. Ahah we're funny."

+		t.Errorf("req returned status %d, want %d", got, want)
+	}
+
+	// We expect only one client call, but three requests through to the server.
+	if got, want := clientCalls, 1; got != want {
+		t.Errorf("handler callback invoked %d times, want %d", got, want)
+	}
+	if got, want := len(rt.Responses), 3; got != want {
+		t.Errorf("len(Responses) = %d, want %d", got, want)
+	}
+
+	// Check that the last response is the one we handled, and that all the bodies
+	// were closed.
+	if got, want := lastResp, rt.Responses[2]; got != want {
+		t.Errorf("Last Response did not match Response in handler callback.\nGot:  %v\nWant: %v", got, want)
+	}
+	for i, resp := range rt.Responses {
+		rc := resp.Body.(*trackingReadCloser)
+		if !rc.Closed {
+			t.Errorf("Reponses[%d].Body was not closed", i)
+		}
+	}
+}
+
+// trackingRoundTripper wraps an http.RoundTripper, keeping track of any
+// returned Responses. Each response's Body, when set, is wrapped with a
+// trackingReadCloser.
+type trackingRoundTripper struct {
+	http.RoundTripper
+	Responses []*http.Response
+	mu        sync.Mutex

[M-A Ruel, 2016/12/12 17:16:39]

switch the order to make it clear what mu protects.

[djd-OOO-Apr2017, 2016/12/13 00:18:09]

Done.

+}
+
+func (t *trackingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	resp, err := t.RoundTripper.RoundTrip(req)
+	if resp != nil && resp.Body != nil {
+		resp.Body = &trackingReadCloser{ReadCloser: resp.Body}
+	}
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	t.Responses = append(t.Responses, resp)
+	return resp, err
+}
+
+// trackingReadCloser wraps an io.ReadCloser, keeping track of whether Closed was
+// called.
+type trackingReadCloser struct {
+	io.ReadCloser
+	Closed bool
+}
+
+func (t *trackingReadCloser) Close() error {
+	t.Closed = true
+	return t.ReadCloser.Close()
+}
+
 // Private details.
 
 func fast() retry.Iterator {