code review 8166045: net/http: ignore 100-continue responses in Transport

net/http: ignore 100-continue responses in Transport

"There are only two hard problems in computer science:
cache invalidation, naming things, and off-by-one errors."

The HTTP server code already strips Expect: 100-continue on
requests, so httputil.ReverseProxy should be unaffected, but
some servers send unsolicited HTTP/1.1 100 Continue responses,
so we need to skip over them if they're seen to avoid getting
off-by-one on Transport requests/responses.

This does change the behavior of people who were using Client
or Transport directly and explicitly setting "Expect: 100-continue"
themselves, but it didn't work before anyway. Now instead of the
user code seeing a 100 response and then things blowing up, now
it basically works, except the Transport will still blast away
the full request body immediately.  That's the part that needs
to be finished to close this issue.

This is the safe quick fix.

Update Issue 3665

[bradfitz, 2013-03-30 00:04:33]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-03-30 00:06:46]

Don't be afraid of the CL size.  It's all test.  The actual fix is tiny:

diff -r 192e257c6507 src/pkg/net/http/transport.go
--- a/src/pkg/net/http/transport.go Fri Mar 29 14:17:09 2013 -0700
+++ b/src/pkg/net/http/transport.go Fri Mar 29 17:06:06 2013 -0700
@@ -686,6 +686,12 @@
  var resp *Response
  if err == nil {
  resp, err = ReadResponse(pc.br, rc.req)
+ if err == nil && resp.StatusCode == 100 {
+ // Issue 2184. Skip unsolicited 100-continue.
+ // TODO: if actually requested, actually signal the caller.
+ // For now we eat it, since we're never expecting one.
+ resp, err = ReadResponse(pc.br, rc.req)
+ }
  }
  hasBody := resp != nil && rc.req.Method != "HEAD" && resp.ContentLength
!= 0




On Fri, Mar 29, 2013 at 5:04 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: ignore 100-continue responses in Transport
>
> "There are only two hard problems in computer science:
> cache invalidation, naming things, and off-by-one errors."
>
> The HTTP server code already strips Expect: 100-continue on
> requests, so httputil.ReverseProxy should be unaffected, but
> some servers send unsolicited HTTP/1.1 100 Continue responses,
> so we need to skip over them if they're seen to avoid getting
> off-by-one on Transport requests/responses.
>
> This does change the behavior of people who were using Client
> or Transport directly and explicitly setting "Expect: 100-continue"
> themselves, but it didn't work before anyway. Now instead of the
> user code seeing a 100 response and then things blowing up, now
> it basically works, except the Transport will still blast away
> the full request body immediately.  That's the part that needs
> to be finished to close this issue.
>
> This is the safe quick fix.
>
> Update Issue 3665
>
> Please review this at
https://codereview.appspot.**com/8166045/<https://codereview.appspot.com/8166...
>
> Affected files:
>   M src/pkg/net/http/transport.go
>   M src/pkg/net/http/transport_**test.go
>
>
>

[dsymonds, 2013-03-30 00:20:43]

LGTM

But drop the second "actually" in the comment.

[dfc, 2013-03-30 00:53:44]

+cc jgc as the CloudFlare folks must hate 100-Continue as much as I do.

[dfc, 2013-03-30 01:10:08]

https://codereview.appspot.com/8166045/diff/4003/src/pkg/net/http/transport_t...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/8166045/diff/4003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:1411: registerPipe := func(pw *io.PipeWriter)
{
I like this local function declaration style you have going on here.

https://codereview.appspot.com/8166045/diff/4003/src/pkg/net/http/transport_t...
src/pkg/net/http/transport_test.go:1487: t.Fatalf("Response Statuscode=%d; want
200 (i=%d): %v", res.StatusCode, i, err)
If I roll back the change to transport.go, then the test fails (as expected),
but also whinges about leaking a transport. I am not sure if that is worth
addressing.

--- FAIL: TestTransportReading100Continue (1.00 seconds)
        transport_test.go:1487: Response Statuscode=100; want 200 (i=1): <nil>
        transport_test.go:1437: EOF
        z_last_test.go:96: Test appears to have leaked a Transport:
                net/http.(*persistConn).readLoop(0xc201e80a00)
                        /home/dfc/go/src/pkg/net/http/transport.go:684 +0x29a
                created by net/http.(*Transport).dialConn
                        /home/dfc/go/src/pkg/net/http/transport.go:511 +0x574

                net/http.(*persistConn).writeLoop(0xc201e80a00)
                        /home/dfc/go/src/pkg/net/http/transport.go:763 +0x26f
                created by net/http.(*Transport).dialConn
                        /home/dfc/go/src/pkg/net/http/transport.go:512 +0x58b
--- FAIL: TestGoroutinesRunning (0.00 seconds)
        z_last_test.go:58: num goroutines = 2
        z_last_test.go:60: Too many goroutines.
        z_last_test.go:62: 1 instances of:
                net/http.(*persistConn).readLoop(0xc201e80a00)
                        /home/dfc/go/src/pkg/net/http/transport.go:684 +0x29a
                created by net/http.(*Transport).dialConn
                        /home/dfc/go/src/pkg/net/http/transport.go:511 +0x574
        z_last_test.go:62: 1 instances of:
                net/http.(*persistConn).writeLoop(0xc201e80a00)
                        /home/dfc/go/src/pkg/net/http/transport.go:763 +0x26f
                created by net/http.(*Transport).dialConn
                        /home/dfc/go/src/pkg/net/http/transport.go:512 +0x58b
FAIL
exit status 1

[bradfitz, 2013-03-30 03:25:19]

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

net/http: ignore 100-continue responses in Transport

"There are only two hard problems in computer science:
cache invalidation, naming things, and off-by-one errors."

The HTTP server code already strips Expect: 100-continue on
requests, so httputil.ReverseProxy should be unaffected, but
some servers send unsolicited HTTP/1.1 100 Continue responses,
so we need to skip over them if they're seen to avoid getting
off-by-one on Transport requests/responses.

This does change the behavior of people who were using Client
or Transport directly and explicitly setting "Expect: 100-continue"
themselves, but it didn't work before anyway. Now instead of the
user code seeing a 100 response and then things blowing up, now
it basically works, except the Transport will still blast away
the full request body immediately.  That's the part that needs
to be finished to close this issue.

This is the safe quick fix.

Update Issue 3665

R=golang-dev, dsymonds, dave, jgrahamc
CC=golang-dev
https://codereview.appspot.com/8166045

[albert.strasheim, 2013-03-30 05:32:51]

Hello

On 2013/03/30 03:25:19, bradfitz wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=466040fd273e ***
> net/http: ignore 100-continue responses in Transport

Seeing quite a few of these on linux/386:

--- FAIL: TestTransportReading100Continue-74 (0.00 seconds)
transport_test.go:1484: Do (i=3): Post http://dummy.tld/: malformed HTTP status
code "/"
transport_test.go:1437: EOF
FAIL

There's also other crashes in net/http due to some GC/runtime issue, but this
looks like something separate from that.

Cheers

Albert

[swhite, 2013-03-30 16:41:39]

LGTM.  This should fix my off-by-one issue on keep-alives with 100's in the
response stream.

[bradfitz, 2013-03-30 16:51:08]

On Fri, Mar 29, 2013 at 10:32 PM, <fullung@gmail.com> wrote:

> Hello
>
>
> On 2013/03/30 03:25:19, bradfitz wrote:
>
>> *** Submitted as
>>
>
https://code.google.com/p/go/**source/detail?r=466040fd273e<https://code.goog...
>
>> net/http: ignore 100-continue responses in Transport
>>
>
> Seeing quite a few of these on linux/386:
>
> --- FAIL: TestTransportReading100Continu**e-74 (0.00 seconds)
> transport_test.go:1484: Do (i=3): Post http://dummy.tld/: malformed HTTP
> status code "/"
> transport_test.go:1437: EOF
> FAIL
>

I can reproduce.

Sorry. One day I'll be able to write a test that works on your systems on
the first try.

Trying to figure out the fix now.