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: Transport: be paranoid about any non-100 1xx response

Since we can't properly handle anything except 100, treat all
1xx informational responses as sketchy and don't reuse the
connection for future requests.

The only other 1xx response code currently in use in the wild
is WebSockets' use of "101 Switching Protocols", but our
code.google.com/p/go.net/websockets doesn't use Client or
Transport: it uses ReadResponse directly, so is unaffected by
this CL. (and its tests still pass)

So this CL is entirely just future-proofing paranoia.
Also: the Internet is weird.

Update Issue 3665

Please review this at https://codereview.appspot.com/8208043/

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


Index: src/pkg/net/http/serve_test.go
===================================================================
--- a/src/pkg/net/http/serve_test.go
+++ b/src/pkg/net/http/serve_test.go
@@ -77,10 +77,15 @@
io.Reader
io.Writer
noopConn
- closec chan bool // if non-nil, send value to it on close
+
+ closeFunc func() error // called if non-nil
+ closec chan bool // else, if non-nil, send value to it on close
}

func (c *rwTestConn) Close() error {
+ if c.closeFunc != nil {
+ return c.closeFunc()
+ }
select {
case c.closec <- true:
default:
Index: src/pkg/net/http/transport.go
===================================================================
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -715,7 +715,10 @@
resp.Body = &bodyEOFSignal{body: resp.Body}
}

- if err != nil || resp.Close || rc.req.Close {
+ if err != nil || resp.Close || rc.req.Close || resp.StatusCode <= 199 {
+ // Don't do keep-alive on error, if either party requested a close,
+ // or we get an unexpected informational (1xx) response.
+ // StatusCode 100 is already handled above.
alive = false
}

Index: src/pkg/net/http/transport_test.go
===================================================================
--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -1416,18 +1416,28 @@
for {
n++
req, err := ReadRequest(br)
+ if err == io.EOF {
+ return
+ }
if err != nil {
t.Error(err)
return
}
slurp, err := ioutil.ReadAll(req.Body)
- if err != nil || string(slurp) != reqBody(n) {
- t.Errorf("Server got %q, %v; want 'body'", slurp, err)
+ if err != nil {
+ t.Errorf("Server request body slurp: %v", err)
return
}
id := req.Header.Get("Request-Id")
+ resCode := req.Header.Get("X-Want-Response-Code")
+ if resCode == "" {
+ resCode = "100 Continue"
+ if string(slurp) != reqBody(n) {
+ t.Errorf("Server got %q, %v; want %q", slurp, err, reqBody(n))
+ }
+ }
body := fmt.Sprintf("Response number %d", n)
- v := []byte(strings.Replace(fmt.Sprintf(`HTTP/1.1 100 Continue
+ v := []byte(strings.Replace(fmt.Sprintf(`HTTP/1.1 %s
Date: Thu, 28 Feb 2013 17:55:41 GMT

HTTP/1.1 200 OK
@@ -1435,7 +1445,7 @@
Echo-Request-Id: %s
Content-Length: %d

-%s`, id, len(body), body), "\n", "\r\n", -1))
+%s`, resCode, id, len(body), body), "\n", "\r\n", -1))
w.Write(v)
if id == reqID(numReqs) {
return
@@ -1451,6 +1461,11 @@
conn := &rwTestConn{
Reader: cr,
Writer: sw,
+ closeFunc: func() error {
+ sw.Close()
+ cw.Close()
+ return nil
+ },
}
go send100Response(cw, sr)
return conn, nil
@@ -1459,20 +1474,37 @@
}
defer tr.CloseIdleConnections()
c := &Client{Transport: tr}
+
+ testResponse := func(req *Request, name string, wantCode int) {
+ res, err := c.Do(req)
+ if err != nil {
+ t.Fatalf("%s: Do: %v", name, err)
+ }
+ if res.StatusCode != wantCode {
+ t.Fatalf("%s: Response Statuscode=%d; want %d", name, res.StatusCode,
wantCode)
+ }
+ if id, idBack := req.Header.Get("Request-Id"),
res.Header.Get("Echo-Request-Id"); id != "" && id != idBack {
+ t.Errorf("%s: response id %q != request id %q", name, idBack, id)
+ }
+ _, err = ioutil.ReadAll(res.Body)
+ if err != nil {
+ t.Fatalf("%s: Slurp error: %v", name, err)
+ }
+ }
+
+ // Few 100 responses, making sure we're not off-by-one.
for i := 1; i <= numReqs; i++ {
req, _ := NewRequest("POST", "http://dummy.tld/",
strings.NewReader(reqBody(i)))
req.Header.Set("Request-Id", reqID(i))
- res, err := c.Do(req)
- if err != nil {
- t.Fatalf("Do (i=%d): %v", i, err)
- }
- if res.StatusCode != 200 {
- t.Fatalf("Response Statuscode=%d; want 200 (i=%d): %v", res.StatusCode,
i, err)
- }
- _, err = ioutil.ReadAll(res.Body)
- if err != nil {
- t.Fatalf("Slurp error (i=%d): %v", i, err)
- }
+ testResponse(req, fmt.Sprintf("100, %d/%d", i, numReqs), 200)
+ }
+
+ // And some other informational 1xx but non-100 responses, to test
+ // we return them but don't re-use the connection.
+ for i := 1; i <= numReqs; i++ {
+ req, _ := NewRequest("POST", "http://other.tld/",
strings.NewReader(reqBody(i)))
+ req.Header.Set("X-Want-Response-Code", "123 Sesame Street")
+ testResponse(req, fmt.Sprintf("123, %d/%d", i, numReqs), 123)
}
}



--

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

  • David Symonds at Mar 31, 2013 at 5:24 am
    LGTM

    though ignoring any error back from NewRequest in the test makes me
    twitch a little.

    --

    ---
    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 31, 2013 at 5:59 am
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=d82ec1b4fb7a ***

    net/http: Transport: be paranoid about any non-100 1xx response

    Since we can't properly handle anything except 100, treat all
    1xx informational responses as sketchy and don't reuse the
    connection for future requests.

    The only other 1xx response code currently in use in the wild
    is WebSockets' use of "101 Switching Protocols", but our
    code.google.com/p/go.net/websockets doesn't use Client or
    Transport: it uses ReadResponse directly, so is unaffected by
    this CL. (and its tests still pass)

    So this CL is entirely just future-proofing paranoia.
    Also: the Internet is weird.

    Update Issue 2184
    Update Issue 3665

    R=golang-dev, dsymonds
    CC=golang-dev
    https://codereview.appspot.com/8208043


    https://codereview.appspot.com/8208043/

    --

    ---
    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 31, '13 at 1:51a
activeMar 31, '13 at 5:59a
posts3
users2
websitegolang.org

2 users in discussion

Bradfitz: 2 posts David Symonds: 1 post

People

Translate

site design / logo © 2022 Grokbase