code review 87550045: net/http: clarify Response.Body Close responsibility

net/http: clarify Response.Body Close responsibility

Per TODO email in my inbox.

[bradfitz, 2014-04-14 20:18:20]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dsymonds@golang.org, r@golang.org, rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2014-04-14 20:20:12]

Doesn't 'done reading from it' imply the rest?

[bradfitz, 2014-04-14 20:22:14]

No.

If I read just that, I could believe that an error from Read was a sign
that the system would clean up after me now, since something went wrong.




On Mon, Apr 14, 2014 at 1:20 PM, Russ Cox <rsc@golang.org> wrote:

> Doesn't 'done reading from it' imply the rest?
>
>
>

[rsc, 2014-04-14 21:02:05]

okay, how about

"It is the caller's responsibility to close the Body."

[rsc, 2014-04-14 21:02:30]

actually, this sentence seems like it is in the wrong place.
this is describing a data structure. there is no 'caller'.

[bradfitz, 2014-04-14 21:11:39]

I want something here because people look here for guidance and people have
asked me because they didn't see it there.

A lot of things returns a Response.  (Post, Get, Head, Client.Do,
Client.Post, Client.Head, Client.Get, RoundTrip)  We could duplicate it on
all of them, or at least do it in one central place.  I prefer at least the
latter.



On Mon, Apr 14, 2014 at 2:02 PM, <rsc@golang.org> wrote:

> actually, this sentence seems like it is in the wrong place.
> this is describing a data structure. there is no 'caller'.
>
>
> https://codereview.appspot.com/87550045/
>

[rsc, 2014-04-14 22:58:08]

https://codereview.appspot.com/87550045/diff/40001/src/pkg/net/http/response.go
File src/pkg/net/http/response.go (right):

https://codereview.appspot.com/87550045/diff/40001/src/pkg/net/http/response....
src/pkg/net/http/response.go:50: // a zero-lengthed body.
s/lengthed/length/

and then put the new thing here, because it has set up the context.

// a zero-length body. It is the caller's responsibility to close Body.

[bradfitz, 2014-04-14 23:01:58]

Hello golang-codereviews@googlegroups.com, rsc@golang.org (cc: adg@golang.org,
dsymonds@golang.org, golang-codereviews@googlegroups.com, r@golang.org),

Please take another look.

[rsc, 2014-04-14 23:48:02]

LGTM

[bradfitz, 2014-04-14 23:50:23]

*** Submitted as https://code.google.com/p/go/source/detail?r=15c88860a91c ***

net/http: clarify Response.Body Close responsibility

Per TODO email in my inbox.

LGTM=rsc
R=golang-codereviews, rsc
CC=adg, dsymonds, golang-codereviews, r
https://codereview.appspot.com/87550045

[gobot, 2014-04-15 00:01:42]

This CL appears to have broken the linux-386-387 builder.
See http://build.golang.org/log/ede7e12362a941d93bf1fe21db9208a3e298029e

[bradfitz, 2014-04-15 00:03:07]

Btw, this failure:

--- FAIL: TestPipeLookPathLeak (0.09 seconds)
exec_test.go:228: leaked 4 fds; want ~0
2014/04/14 23:59:28 http: TLS handshake error from 127.0.0.1:36251: remote
error: bad certificate
FAIL
FAIL os/exec 0.981s

Is about that "leaked 4 fds" line.  The line after it is expected noise.  I
can shut it up.

But there's still a leak.



On Mon, Apr 14, 2014 at 5:01 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the linux-386-387 builder.
> See http://build.golang.org/log/ede7e12362a941d93bf1fe21db9208a3e298029e
>
> https://codereview.appspot.com/87550045/
>