code review 906042: Handle HTTP body properly when a HTTP/1.1 response o...

http: fix handling of HTTP/1.1 response body.

        Certain response codes always have a body, even
        if Content-Length and Transfer-Encoding are omitted.
	Fixes issue 716.

[dsymonds, 2010-04-09 04:57:09]

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

I'd like you to review this change.

[rsc, 2010-04-09 05:59:33]

Doesn't the determination of whether there is a body
have to depend on the response code?
I'd like to see a test of the same input but s/200/204/
making sure it does *not* read the "Body here" lines.

Please start the description with a single line
description beginning with "http:" to identify the
package, like

---
http: fix handling of HTTP/1.1 response body

Certain response codes always have a body, even
if Content-Length and Transfer-Encoding are omitted.
---

Also, do you know why the description is indented
funny in codereview?  I've seen this before and
I think it has something to do with editors screwing
up the template, but I don't know how.  Maybe your
editor changed the leading tabs to spaces and that
matters?

Thanks.
Russ

[dsymonds, 2010-04-09 06:22:53]

All done, code-wise. PTAL.

On Fri, Apr 9, 2010 at 3:59 PM, Russ Cox <rsc@golang.org> wrote:

> Also, do you know why the description is indented
> funny in codereview?  I've seen this before and
> I think it has something to do with editors screwing
> up the template, but I don't know how.  Maybe your
> editor changed the leading tabs to spaces and that
> matters?

Yeah, I don't know what happened. I think I've fixed it now.


Dave.

[rsc1, 2010-04-09 07:02:38]

Thanks, but I think there's another problem.
It's pretty subtle code, regardless.

+petar-m for answer to question below.

http://codereview.appspot.com/906042/diff/9002/11003
File src/pkg/http/transfer.go (right):

http://codereview.appspot.com/906042/diff/9002/11003#newcode220
src/pkg/http/transfer.go:220: // If there is no Content-Length or chunked
Transfer-Encoding,
This is only true for *Response.
And for reasons I don't really understand, t.StatusCode will be 200 for a
*Request.

Please add a test in request_test.go that a GET without these headers
doesn't try to have a body.

Petar, would it make more sense to make t.StatusCode == 0 and 
t.RequestMethod == "" for a *Request?

[petar-m, 2010-04-09 16:33:42]

This change will need an overall revision. See my comments.

http://codereview.appspot.com/906042/diff/9002/11003
File src/pkg/http/transfer.go (right):

http://codereview.appspot.com/906042/diff/9002/11003#newcode220
src/pkg/http/transfer.go:220: // If there is no Content-Length or chunked
Transfer-Encoding,
Request.StatusCode = 200 in order to liken it to a Response which *may*
have a body (POST requests have bodies). This was targeting precisely
the test for StatusCode != 1xx,204,304.

It should be fine to make Request.StatusCode any number other 
than those up there. I would recommend -1, for safety.

The field RequestMethod (for a Request) is GET in order to
pass the noBodyExpected() test in fixLength(). Again, this is
trying to liken a request to a response that MAY have a body.
Setting it to "" should be fine.


On 2010/04/09 07:02:38, rsc1 wrote:
> This is only true for *Response.
> And for reasons I don't really understand, t.StatusCode will be 200 for a
> *Request.
> 
> Please add a test in request_test.go that a GET without these headers
> doesn't try to have a body.
> 
> Petar, would it make more sense to make t.StatusCode == 0 and 
> t.RequestMethod == "" for a *Request?
>

http://codereview.appspot.com/906042/diff/9002/11003#newcode223
src/pkg/http/transfer.go:223: _, hasCL := t.Header["Content-Length"]
Don't test for ContentLength like this. Use t.ContentLength. Look at fixLength()
for its meaning. In fact a lot of your code here duplicates with fixLength().

http://codereview.appspot.com/906042/diff/9002/11003#newcode224
src/pkg/http/transfer.go:224: te, hasTE := t.Header["Transfer-Encoding"]
Don't test for chunked encoding like this. Use "chunked(t.TransferEncoding)"
(see example a few lines below in the code). This refers to the line above the
comment.

http://codereview.appspot.com/906042/diff/9002/11003#newcode229
src/pkg/http/transfer.go:229: }
The bottom line is that this entire change should not be here, it should be in
fixLength. And from the looks of it, all that must be done is:

For requests, initialize:

StatusCode = -1
RequestMethod = ""

and then make the respective modification in fixLength(),
which should be quite minimal.

Did you actually encounter a situation where the current code 
misbehaves?

http://codereview.appspot.com/906042/diff/9002/11003#newcode241
src/pkg/http/transfer.go:241: if t.Close || unboundedBody {
This is redundant (and thus confusing). You don't need the || part
since you already set t.Close = true when unboundedBody = true.

http://codereview.appspot.com/906042/diff/9002/11003#newcode250
src/pkg/http/transfer.go:250: // cases?). This logic should be in fixLength().
Did you notice this comment above? All of your changes should be in fixLength().
This function here is the "master logic" and should stay relatively clean.

[rsc, 2011-02-09 05:44:42]

removing reviewers due to bradfitz's CL

[dsymonds, 2011-02-10 03:32:56]

*** Abandoned ***