net/http: add clear documentation on when to drain & close a Response

[daenney]

This is more of a documentation request / proposal to add something to the language FAQ.

There seems to currently be no authoritative answer to the following question: Do you need to drain and close an http.Response, and if so do you always need to do this or only under certain circumstances?

Throughout my own journey in Go I've seen answers varying from "no" to "only if the request is over Xkb", "always" and a lot of things in between. Doing this wrongly can lead to resource leaks so it would seem nice to at least know what the state of this is. It also seems the answer to this may have changed over time but the response tends to be "read net/http" which isn't very practical and is something one would have to redo for every Go release. The complexity of "net/http" has grown over time too and some parts of it aren't all that approachable.

I bring this up because this post is currently making the rounds: https://manishrjain.com/must-close-golang-http-response. In it, the author suggests a few things:

• Response body must always be closed
• Response body must always be drained and closed
• Not using the DefaultTransport makes a difference in this behaviour

From the Reddit discussion, we also see a number of confused people, the advice to drain, to drain with a limit reader and that as long as you read the whole response body it is implicitly closed so you don't need to call Close().

That particular post and the ensuing confusion in the discussion including contradicting advice is only a recent example, there's a lot more of this.

It would be nice if what is expected here could be authoritatively stated in the examples & documentation of the net/http package. This seems to be a rather common issue and potential foot-gun in Go. Given Go is used extensively to write networked services, resolving ambiguity around what the client needs to do would be helpful.

The fact that this seems to confuse people to this day would also suggest http.Response might be overdue for a convenience helper that drains and/or closes the body that you can safely defer and forget about.

[ianlancetaylor]

No reason for this to be a proposal, so taking it out of the proposal process.

[ianlancetaylor]

CC @neild

[neild]

The third sentence of the net/http documentation is, "The client must close the response body when finished with it."

The Response.Body documentation states, "It is the caller's responsibility to close Body."

The Client.Get documentation states, "When err is nil, resp always contains a non-nil resp.Body. Caller should close resp.Body when done reading from it." Other Client methods have similar text.

It's clear that there's still confusion here, but a concrete suggestion on how the documentation would be improved would help. We already state this in multiple places: The user must always close the response body, without exception.

It is not necessary to drain the body first.

[chrisguiney]

@neild

for the Client.Do documentation, it states:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

I think this blurb is tripping people up.

I think it may be appropriate to add some ReadAndClose(*Response) error function that everyone can point to for "doing the right thing" most of the time.

func pingServer() error {
  var (
    resp *http.Response
    err error
  )
  resp, err = client.Get("server-i-contact-frequently.com")
  if err != nil {
      return fmt.Errorf("failed issuing request: %w", err)
  }
  defer func() {
    err = errors.Join(err, resp.ReadAndClose())
  }()

  // continue to use response
}

as opposed to

func pingServer() error {
  var (
    resp *http.Response
    err error
  )
  resp, err = client.Get("server-i-contact-frequently.com")
  if err != nil {
      return fmt.Errorf("failed issuing request: %w", err)
  }
  
  defer func() {
    defer func() {
      err = errors.Join(err, resp.Close())
    }()

    // isn't technically necessary, but makes connection re-usable
    r := io.LimitReader(resp.Body, 10*1024*1024) // protect against some unexpected response size
    if n, cErr := io.Copy(io.Discard, r); cErr != nil {
       err = errors.Join(err,  fmt.Errorf("failed reading response body after %d bytes: %w", n, cErr))
    }
  }()
  
  // continue to use response
}

[daenney]

Yeah, I think that's the crux of it. The closing of Response.Body() seems to be overall properly understood, though there's apparently some idea that reading Response.Body() fully implicitly closes it?

Then Client.Do() comes in which confuses things a bit, and I think a comment on Response.Body is also confusing things

/ 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.

This seems to be where people are getting the idea from that as long as you don't use DefaultTransport, your transport will actually reuse HTTP/1.x "keep-alive" connections (potentially regardless of body draining and closing).

[neild]

I think it may be appropriate to add some ReadAndClose(*Response) error function that everyone can point to for "doing the right thing" most of the time.

The right thing should be to call Response.Body.Close. If that's not the right thing, then we should fix it, but we shouldn't be adding more API surface for people to be confused about.

I would not recommend going out of your way to drain response bodies unless you've got a specific and measurable performance issue that you're trying to resolve.

[chrisguiney]

If that's not the right thing, then we should fix it, but we shouldn't be adding more API surface for people to be confused about.

If you think that changing the behavior of Resopnse.Body.Close to make it discard the body is a viable solution given the go compatibility promise, that would certainly be an option.

In practice I think it does violate the compatibility promise. It introduces other issues could dramatically impact existing programs. for example how much of a body to read - leaving this unbounded allows a server to periodically send data and keep a connection held open.

I would not recommend going out of your way to drain response bodies unless you've got a specific and measurable performance issue that you're trying to resolve.

connection setup & teardown is a non-trivial cost to both client and server. it may not be an issue for small projects, but i don't think it would be uncommon for a go program to be impacted by the cost -- keep-alives are implemented for a reason.

@neild do you have specific any suggestions on how to solve the problem? are you content with the current situation?

[neild]

The net/http server reads up to 256k from an unread request body after a handler returns, in hopes of continuing to use the connection: https://go.googlesource.com/go/+/refs/tags/go1.20.4/src/net/http/server.go#1089

It might be reasonable for the client to do the same. Or perhaps not; I don't know that this would be a violation of compatibility, but it would be a user-visible change. Perhaps there should be a configuration setting for how much read-ahead we might perform. It would be useful to have some data on how much of an issue this is in practice before trying to fix it.

This issue is about documentation, however. I'm not sure what we can do to improve the documentation. As I said, it's clear that people are confused, but we already document the requirement to close request bodies in many places. How can we be clearer?

[daenney]

The net/http server reads up to 256k from an unread request body after a handler returns, in hopes of continuing to use the connection: https://go.googlesource.com/go/+/refs/tags/go1.20.4/src/net/http/server.go#1089

Ah, that's very interesting. I think this is why I've seen the advice previously that you don't have to drain the body on connection reuse. It's meant to only apply server side, but through all the discussions and remixes of the information that distinction may have gotten lost.

This issue is about documentation, however. I'm not sure what we can do to improve the documentation. As I said, it's clear that people are confused, but we already document the requirement to close request bodies in many places. How can we be clearer?

From my point of view, I think it would be more helpful to have some package level documentation on net/http that spells this out, instead of people needing to puzzle this together by looking at Response, Client.Get, Client.Do etc. It already has a comment about the fact that the client must close the response body which is one part of this issue. I think it might help to expand that section with some explicit guidance on why you may want connection draining on the client side and how to implement that.

Perhaps we can include a second example, that explicitly states: "Aside from always closing the Body, if you want to enable connection reuse you also must ensure you've read the response body. In order to protect against abnormally big response sizes, it's recommended to use an io.LimitReader for this purpose". Follow up with an example that shows a defer func() { io.Discard(...)}() that does this.

It might even be feasible to update the existing example, as I don't think reading the closed body causes an issue? So (attempting) draining the connection client side is probably always safe to do?