net/http: Transport should close body on all errors and match Client.Do docs

[odeke-em]

I just saw CL https://go-review.googlesource.com/c/go/+/202237 by @bored-engineer (thank you Luke!) and was going to review and recommend/create a test for the CL creator.

The docs for http.Client.Do https://golang.org/pkg/net/http/#Client.Do promise that we'll always close the body even on errors and so far we missed a few spots on the trivial cases with minor oversight

go/src/net/http/transport.go

Lines 469 to 495 in 72275c0

	if isHTTP {
	for k, vv := range req.Header {
	if !httpguts.ValidHeaderFieldName(k) {
	return nil, fmt.Errorf("net/http: invalid header field name %q", k)
	}
	for _, v := range vv {
	if !httpguts.ValidHeaderFieldValue(v) {
	return nil, fmt.Errorf("net/http: invalid header field value %q for key %v", v, k)
	}
	}
	}
	}
	if t.useRegisteredProtocol(req) {
	altProto, _ := t.altProto.Load().(map[string]RoundTripper)
	if altRT := altProto[scheme]; altRT != nil {
	if resp, err := altRT.RoundTrip(req); err != ErrSkipAltProtocol {
	return resp, err
	}
	}
	}
	if !isHTTP {
	req.closeBody()
	return nil, &badStringError{"unsupported protocol scheme", scheme}
	}
	if req.Method != "" && !validMethod(req.Method) {
	return nil, fmt.Errorf("net/http: invalid method %q", req.Method)

that is:
a) if isHTTP {
b) if t.useRegisteredProtocol(req)
c) if req.Method != "" && !validMethod(req.Method) {

and I realize this is because that code has been modified piecemeal over the past decade.

This test is what we can use to bootstrap checks

package main

import (
	"io"
	"net/http"
	"net/http/httptest"
	"net/url"
	"testing"
)

type bodyCloser bool

func (bc *bodyCloser) Close() error {
	*bc = true
        return nil
}

func (bc *bodyCloser) Read(b []byte) (n int, err error) {
	return 0, io.EOF
}

func TestInvalidMethodClosesBody(t *testing.T) {
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
	defer cst.Close()
	var bc bodyCloser

	u, _ := url.Parse(cst.URL)
	req := &http.Request{
		Method: " ",
		URL:    u,
		Body:   &bc,
	}

	_, err := http.DefaultClient.Do(req)
	if err == nil {
		t.Fatal("Expected an error")
	}
	if !bc {
		t.Fatal("Expected body to have been closed")
	}
}

which currently gives

$ go test
--- FAIL: TestInvalidMethodClosesBody (0.00s)
    it_test.go:39: Expected body to have been closed

This issue will be to send CLs to enforce those closes as well as tests so that we don't regress.

[gopherbot]

Change https://golang.org/cl/202237 mentions this issue: net/http: ensure request body is closed when method is invalid

[gopherbot]

Change https://golang.org/cl/202239 mentions this issue: net/http: make Transport.RoundTrip close body on any invalid request

[bored-engineer]

@odeke-em what do you think about the remaining useRegisteredProtocol case where an error is returned by the underlying protocol handler. In my opinion Transport shouldn't handle that, it should be the responsibility of the protocol handler as the handler could have already closed the request body before an error occurred. In that case I think updating the docs so that expectation is clear would be a good addition.

[odeke-em]

Thanks for the question Luke! So in the follow-up CL I didn’t touch that either as it is the responsibility of the alternative transport protocol. Sure, perhaps we could update the docs but finding the right wording is tricky because there are the first conditions for which an error will result in a req.closeBody() regardless of the transport/protocol but also it requires referencing Transport.RegisterProtocol, at which point I think the prose might cause more confusion. Perhaps we could add to the line

If an alternative protocol and RoundTripper was registered with the

Transport, it is the responsibility of that RoundTripper to close the body. But am hesitant to add more words. /cc @bradfitz

…

On Sun, Oct 20, 2019 at 8:45 PM Luke Young ***@***.***> wrote: @odeke-em <https://github.com/odeke-em> what do you think about the remaining useRegisteredProtocol case where an error is returned by the underlying protocol handler. In my opinion Transport shouldn't handle that, it should be the responsibility of the protocol handler as the handler could have already closed the request body before an error occurred. In that case I think updating the docs so that expectation is clear would be a good addition. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35015?email_source=notifications&email_token=ABFL3VZERH2YXHGWCY2UOU3QPUQVNA5CNFSM4JCSPOTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBY6ZDY#issuecomment-544337039>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V3BR65YC6R2W3W63IDQPUQVNANCNFSM4JCSPOTA> .