FAQ
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/

Affected files:
M src/pkg/net/http/transport.go
M src/pkg/net/http/transport_test.go


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Brad Fitzpatrick at Mar 30, 2013 at 12:06 am
    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, 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/8166045/>

    Affected files:
    M src/pkg/net/http/transport.go
    M src/pkg/net/http/transport_**test.go

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • David Symonds at Mar 30, 2013 at 12:20 am
    LGTM

    But drop the second "actually" in the comment.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Dave at Mar 30, 2013 at 12:53 am
    +cc jgc as the CloudFlare folks must hate 100-Continue as much as I do.



    https://codereview.appspot.com/8166045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Dave at Mar 30, 2013 at 1:10 am
    https://codereview.appspot.com/8166045/diff/4003/src/pkg/net/http/transport_test.go
    File src/pkg/net/http/transport_test.go (right):

    https://codereview.appspot.com/8166045/diff/4003/src/pkg/net/http/transport_test.go#newcode1411
    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_test.go#newcode1487
    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

    https://codereview.appspot.com/8166045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Bradfitz at Mar 30, 2013 at 3:25 am
    *** 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


    https://codereview.appspot.com/8166045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Fullung at Mar 30, 2013 at 5:32 am
    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

    https://codereview.appspot.com/8166045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Brad Fitzpatrick at Mar 30, 2013 at 4:51 pm

    On Fri, Mar 29, 2013 at 10:32 PM, 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.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: 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.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedMar 30, '13 at 12:04a
activeMar 30, '13 at 4:51p
posts8
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase