net/http: checks for nil Response or Response.Body in Client.send causing some internal Google tests to fail

[andybons]

With https://golang.org/cl/221818, we do explicit checks to see if a Response from a RoundTripper is nil or has a nil Body, returning an error if so. This is causing some test code to fail. A contrived example:

type fakeTransport func(*http.Request) (*http.Response, error)

func (f fakeTransport) RoundTrip(req *http.Request) (*http.Response, error) {
  return f(req)
}

...

func TestFoo(t *testing.T) {
  transport := fakeTransport(func(req *http.Request) (*http.Response, error) {
    return &http.Response{StatusCode: http.StatusOK}, nil
  })
  client := &http.Client{Transport: transport}
  // use client, get panic
  ...
}

It’s documented that http.Client.Do will always return a non-nil Body if its error is nil.

Should we be enforcing a non-nil Body in these scenarios? My general feeling is yes, though creating a NopCloser body pulls in ioutil and likely another package like strings or bytes for their NewReader functions in these cases.

Thoughts, @bcmills?

[bcmills]

Reading the package docs, perhaps we could relax this check.

https://golang.org/pkg/net/http/#Response says (emphasis mine):

  // The http Client and Transport guarantee that Body is always
  // non-nil, even on responses without a body or responses with
  // a zero-length body. It is the caller's responsibility to
  // close Body. The default HTTP client's Transport may not
  // reuse HTTP/1.x "keep-alive" TCP connections if the Body is
  // not read to completion and closed.
  

But https://golang.org/pkg/net/http/#RoundTripper says:

 // … Callers
 // should not mutate or reuse the request until the Response's
 // Body has been closed.

which to me implies a non-nil Body. But perhaps it could be construed to mean “until the Response's body has been closed if non-nil”.

[andybons]

Whatever you think is best.

[ianlancetaylor]

Also CC @bradfitz

[gopherbot]

Change https://golang.org/cl/225717 mentions this issue: net/http: treat a nil Body from a custom RoundTripper as an empty one