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

CC @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.

@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
}

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

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.

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?

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?

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?