code review 4529050: http: fix transport bug with zero-length bodies

http: fix transport bug with zero-length bodies

An optimization in Transport which re-uses TCP
connections early in the case where there is
no response body interacted poorly with
ErrBodyReadAfterClose.  Upon recycling the TCP
connection early we would Close the Response.Body
(in case the user forgot to), but in the case
of a zero-lengthed body, the user's handler might
not have run yet.

This CL makes sure the Transport doesn't try
to Close requests when we're about to immediately
re-use the TCP connection.

This also includes additional tests I wrote
while debugging.

[bradfitz, 2011-05-11 11:57:47]

Hello rsc (cc: golang-dev@googlegroups.com),

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

[rsc, 2011-05-11 14:35:50]

http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go
File src/pkg/http/persist.go (right):

http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go#newco...
src/pkg/http/persist.go:373: // We don't call close on a 'noBodyReader', though,
because a) we don't need
Why bother?  Why not just make noBodyReader's Close a no-op?
Then this code doesn't need to know anything about transfer.go's details.

[bradfitz, 2011-05-11 17:11:32]

I didn't want to change the behavior from the handler's/clients's point of
view. The API shouldn't differ just slightly depending on the response.

I can do this differently if you think touching persist.go is gross but I
don't want to change the API. But I figured persist changes were in private
details, all part of same package. I could make transport smarter instead.
On May 11, 2011 7:35 AM, <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go
> File src/pkg/http/persist.go (right):
>
>
http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go#newco...
> src/pkg/http/persist.go:373: // We don't call close on a 'noBodyReader',
> though, because a) we don't need
> Why bother? Why not just make noBodyReader's Close a no-op?
> Then this code doesn't need to know anything about transfer.go's
> details.
>
> http://codereview.appspot.com/4529050/

[rsc, 2011-05-11 17:33:38]

but you're not changing the API.
do you mean that the client might
not get the Read error after a Close?
that's removing a debugging error check
from one path, a path that doesn't
actually matter since there is nothing to read.
that doesn't seem like changing the API to me.

On Wed, May 11, 2011 at 13:11, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I didn't want to change the behavior from the handler's/clients's point of
> view. The API shouldn't differ just slightly depending on the response.
>
> I can do this differently if you think touching persist.go is gross but I
> don't want to change the API. But I figured persist changes were in private
> details, all part of same package. I could make transport smarter instead.
>
> On May 11, 2011 7:35 AM, <rsc@golang.org> wrote:
>>
>> http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go
>> File src/pkg/http/persist.go (right):
>>
>>
>>
http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go#newco...
>> src/pkg/http/persist.go:373: // We don't call close on a 'noBodyReader',
>> though, because a) we don't need
>> Why bother? Why not just make noBodyReader's Close a no-op?
>> Then this code doesn't need to know anything about transfer.go's
>> details.
>>
>> http://codereview.appspot.com/4529050/
>

[bradfitzgoog, 2011-05-11 17:45:39]

Read shouldn't work after Close. I don't want to sometimes allow it and
sometimes not. I'd rather go out of our way to be consistent.

What does read after close do on other io.Readers?

(phone), brad
On May 11, 2011 10:33 AM, "Russ Cox" <rsc@golang.org> wrote:
> but you're not changing the API.
> do you mean that the client might
> not get the Read error after a Close?
> that's removing a debugging error check
> from one path, a path that doesn't
> actually matter since there is nothing to read.
> that doesn't seem like changing the API to me.
>
> On Wed, May 11, 2011 at 13:11, Brad Fitzpatrick <bradfitz@golang.org>
wrote:
>> I didn't want to change the behavior from the handler's/clients's point
of
>> view. The API shouldn't differ just slightly depending on the response.
>>
>> I can do this differently if you think touching persist.go is gross but I
>> don't want to change the API. But I figured persist changes were in
private
>> details, all part of same package. I could make transport smarter
instead.
>>
>> On May 11, 2011 7:35 AM, <rsc@golang.org> wrote:
>>>
>>> http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go
>>> File src/pkg/http/persist.go (right):
>>>
>>>
>>>
http://codereview.appspot.com/4529050/diff/4001/src/pkg/http/persist.go#newco...
>>> src/pkg/http/persist.go:373: // We don't call close on a 'noBodyReader',
>>> though, because a) we don't need
>>> Why bother? Why not just make noBodyReader's Close a no-op?
>>> Then this code doesn't need to know anything about transfer.go's
>>> details.
>>>
>>> http://codereview.appspot.com/4529050/
>>

[rsc, 2011-05-11 17:48:30]

On Wed, May 11, 2011 at 13:45, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Read shouldn't work after Close. I don't want to sometimes allow it and
> sometimes not. I'd rather go out of our way to be consistent.
>
> What does read after close do on other io.Readers?

It depends on how much they care.  In an ioutil.NopCloser
you can Read after Close.  Most others actually close things,
so read will fail.

Russ

[rsc, 2011-05-11 17:49:03]

On Wed, May 11, 2011 at 13:45, Brad Fitzpatrick <bradfitz@google.com> wrote:
> Read shouldn't work after Close. I don't want to sometimes allow it and
> sometimes not. I'd rather go out of our way to be consistent.

i guess; reading 0 bytes successfully after close does not bother me.

[bradfitz, 2011-05-11 19:02:08]

Hello rsc, bradfitzgoog (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-05-11 19:03:50]

LGTM

[bradfitz, 2011-05-11 19:11:40]

*** Submitted as http://code.google.com/p/go/source/detail?r=2887dd36b6f8 ***

http: fix transport bug with zero-length bodies

An optimization in Transport which re-uses TCP
connections early in the case where there is
no response body interacted poorly with
ErrBodyReadAfterClose.  Upon recycling the TCP
connection early we would Close the Response.Body
(in case the user forgot to), but in the case
of a zero-lengthed body, the user's handler might
not have run yet.

This CL makes sure the Transport doesn't try
to Close requests when we're about to immediately
re-use the TCP connection.

This also includes additional tests I wrote
while debugging.

R=rsc, bradfitzgoog
CC=golang-dev
http://codereview.appspot.com/4529050