FAQ
Reviewers: golang-dev_googlegroups.com, brainman,

Message:
Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

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


Description:
net/http: handle 413 responses more robustly

When HTTP bodies were too large and we didn't want to finish
reading them for DoS reasons, we previously found it necessary
to send a FIN and then pause before closing the connection
(which might send a RST) if we wanted the client to have a
better chance at receiving our error response. That was Issue 3595.

This issue adds the same fix to request headers which
are too large, which might fix the Windows flakiness
we observed on TestRequestLimit at:
http://build.golang.org/log/146a2a7d9b24441dc14602a1293918191d4e75f1

Please review this at http://codereview.appspot.com/6826084/

Affected files:
M src/pkg/net/http/server.go


Index: src/pkg/net/http/server.go
===================================================================
--- a/src/pkg/net/http/server.go
+++ b/src/pkg/net/http/server.go
@@ -579,13 +579,26 @@
}
}

+// rstAvoidanceDelay is the amount of time we sleep after closing the
+// write side of a TCP connection before closing the entire socket.
+// By sleeping, we increase the chances that the client sees our FIN
+// and processes its final data before they process the subsequent RST
+// from closing a connection with known unread data.
+// This RST seems to occur mostly on BSD systems. (And Windows?)
+// This timeout is somewhat arbitrary (~latency around the planet).
+const rstAvoidanceDelay = 500 * time.Millisecond
+
// closeWrite flushes any outstanding data and sends a FIN packet (if
client
-// is connected via TCP), signalling that we're done.
-func (c *conn) closeWrite() {
+// is connected via TCP), signalling that we're done. We then pause for a
bit,
+// hoping the client processes it any subsequent RST.
+//
+// See http://golang.org/issue/3595
+func (c *conn) closeWriteAndWait() {
c.finalFlush()
if tcp, ok := c.rwc.(*net.TCPConn); ok {
tcp.CloseWrite()
}
+ time.Sleep(rstAvoidanceDelay)
}

// Serve a new connection.
@@ -618,20 +631,21 @@
for {
w, err := c.readRequest()
if err != nil {
- msg := "400 Bad Request"
if err == errTooLarge {
// Their HTTP client may or may not be
// able to read this if we're
// responding to them and hanging up
// while they're still writing their
// request. Undefined behavior.
- msg = "413 Request Entity Too Large"
+ io.WriteString(c.rwc, "HTTP/1.1 413 Request Entity Too Large\r\n\r\n")
+ c.closeWriteAndWait()
+ break
} else if err == io.EOF {
break // Don't reply
} else if neterr, ok := err.(net.Error); ok && neterr.Timeout() {
break // Don't reply
}
- fmt.Fprintf(c.rwc, "HTTP/1.1 %s\r\n\r\n", msg)
+ io.WriteString(c.rwc, "HTTP/1.1 400 Bad Request\r\n\r\n")
break
}

@@ -685,18 +699,7 @@
w.finishRequest()
if w.closeAfterReply {
if w.requestBodyLimitHit {
- // Flush our response and send a FIN packet and wait a bit
- // before closing the connection, so the client has a chance
- // to read our response before they possibly get a RST from
- // our TCP stack from ignoring their unread body.
- // See http://golang.org/issue/3595
- c.closeWrite()
- // Now wait a bit for our machine to send the FIN and the client's
- // machine's HTTP client to read the request before we close
- // the connection, which might send a RST (on BSDs, at least).
- // 250ms is somewhat arbitrary (~latency around half the planet),
- // but this doesn't need to be a full second probably.
- time.Sleep(250 * time.Millisecond)
+ c.closeWriteAndWait()
}
break
}

Search Discussions

  • Alex Brainman at Nov 12, 2012 at 6:35 am
    I do not think it will help any. But, I can't see it hurt either.

    The implementation LGTM. It is the idea I do not like. I think reset is
    fine in these situations.

    Alex

    http://codereview.appspot.com/6826084/
  • Brad Fitzpatrick at Nov 12, 2012 at 4:16 pm

    On Sun, Nov 11, 2012 at 10:35 PM, wrote:

    I do not think it will help any.

    I think it will: it'll give some HTTP clients (those that read while
    writing, like ours) a chance to handle the error before the RST arrives.
    This worked before, in our POST-body-too-large case. It should work here
    too. This is just another case of too much data arriving and us rejecting
    it. We should reject it in the same way.

    But, I can't see it hurt either.
    The implementation LGTM. It is the idea I do not like. I think reset is
    fine in these situations.
    Reset still comes, but I want to increase the chance that HTTP clients see
    errors, like we did before.
  • Ingo Oeser at Nov 12, 2012 at 8:30 pm
    Could you make the timeout configurable? E.g. via Transport?

    Rationale: It could be too big for api calls in the same or a near datacenter and too small for mobile devices.

    But I can surely send a followup change, if you are too busy.
  • Brad Fitzpatrick at Nov 12, 2012 at 11:27 pm

    On Mon, Nov 12, 2012 at 12:30 PM, Ingo Oeser wrote:

    Could you make the timeout configurable? E.g. via Transport?

    Rationale: It could be too big for api calls in the same or a near
    datacenter and too small for mobile devices.

    But I can surely send a followup change, if you are too busy.
    I don't understand, so yeah... I'll let you propose something. But I
    already think I won't like it. I don't see what it has to do with a
    Transport, for instance.

    It doesn't matter if a mobile phone's latency is over 500 ms. This just
    gives the FIN a 500 ms head start. That gives the phone 500 ms to receive
    it and give it to userspace before the RST arrives, even if that's 2
    seconds later from the server's perspective.
  • Ingo Oeser at Nov 12, 2012 at 11:35 pm
    Ah ok, so the intent is fixing up missing synchronization between sending
    and receiving part on the client side via sleeping? This sounds fishy to me
    at least, but often works long enough in practise.

    Another idea might be to close only one side of the tcp duplex, not both.

    But lets fix this up in a follow up, if you feel very confident about this
    change.
    On Nov 13, 2012 12:27 AM, "Brad Fitzpatrick" wrote:
    On Mon, Nov 12, 2012 at 12:30 PM, Ingo Oeser wrote:

    Could you make the timeout configurable? E.g. via Transport?

    Rationale: It could be too big for api calls in the same or a near
    datacenter and too small for mobile devices.

    But I can surely send a followup change, if you are too busy.
    I don't understand, so yeah... I'll let you propose something. But I
    already think I won't like it. I don't see what it has to do with a
    Transport, for instance.

    It doesn't matter if a mobile phone's latency is over 500 ms. This just
    gives the FIN a 500 ms head start. That gives the phone 500 ms to receive
    it and give it to userspace before the RST arrives, even if that's 2
    seconds later from the server's perspective.
  • Brad Fitzpatrick at Nov 12, 2012 at 11:51 pm

    On Mon, Nov 12, 2012 at 3:35 PM, Ingo Oeser wrote:

    Ah ok, so the intent is fixing up missing synchronization between sending
    and receiving part on the client side via sleeping? This sounds fishy to me
    at least, but often works long enough in practise.

    Another idea might be to close only one side of the tcp duplex, not both.
    That's what it does already. I'd start by reading the diff. It has lots
    of comments. Also see the bug it references.
  • Rsc at Nov 12, 2012 at 8:54 pm
  • Bradfitz at Nov 12, 2012 at 11:20 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=e6b3e4d247ef ***

    net/http: handle 413 responses more robustly

    When HTTP bodies were too large and we didn't want to finish
    reading them for DoS reasons, we previously found it necessary
    to send a FIN and then pause before closing the connection
    (which might send a RST) if we wanted the client to have a
    better chance at receiving our error response. That was Issue 3595.

    This issue adds the same fix to request headers which
    are too large, which might fix the Windows flakiness
    we observed on TestRequestLimit at:
    http://build.golang.org/log/146a2a7d9b24441dc14602a1293918191d4e75f1

    R=golang-dev, alex.brainman, rsc
    CC=golang-dev
    http://codereview.appspot.com/6826084


    http://codereview.appspot.com/6826084/
  • Alex Brainman at Nov 12, 2012 at 11:23 pm

    On 2012/11/12 16:16:00, bradfitz wrote:
    On Sun, Nov 11, 2012 at 10:35 PM,
    wrote:
    I do not think it will help any.
    I think it will: ...
    You are right. I am wrong. It does help here. This test:

    diff --git a/src/pkg/net/net_test.go b/src/pkg/net/net_test.go
    --- a/src/pkg/net/net_test.go
    +++ b/src/pkg/net/net_test.go
    @@ -213,3 +213,85 @@
    t.Fatal(err)
    }
    }
    +
    +func TestALEX(t *testing.T) {
    + l, err := Listen("tcp", "127.0.0.1:0")
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer l.Close()
    +
    + read := func(r io.Reader) error {
    + var m [1]byte
    + _, err := r.Read(m[:])
    + return err
    + }
    +
    + write := func(w io.Writer) error {
    + var m [1]byte
    + _, err := w.Write(m[:])
    + return err
    + }
    +
    + ec := make(chan error)
    +
    + // server
    + go func() (err error) {
    + defer func() {
    + ec <- err
    + }()
    +
    + c, err := l.Accept()
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer c.Close()
    +
    + err = read(c)
    + if err != nil {
    + return err
    + }
    + err = write(c)
    + if err != nil {
    + return err
    + }
    +
    +// err = c.(*TCPConn).CloseWrite()
    + if err != nil {
    + return err
    + }
    + return nil
    + }()
    +
    + // client
    + c, err := Dial("tcp", l.Addr().String())
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer c.Close()
    +
    + go func() {
    + var m [1000]byte
    + for {
    + _, err := c.Write(m[:])
    + if err != nil {
    + return
    + }
    + }
    + }()
    +
    + for {
    + err := read(c)
    + if err != nil {
    + if err == io.EOF {
    + break
    + }
    + t.Fatal(err)
    + }
    + }
    +
    + err = <-ec
    + if err != nil {
    + t.Fatal(err)
    + }
    +}

    fails on linux

    # go test -v -run=ALEX
    === RUN TestALEX
    --- FAIL: TestALEX (0.00 seconds)
    net_test.go:289: read tcp 127.0.0.1:38968: connection reset by
    peer
    FAIL
    exit status 1
    FAIL net 0.009s

    and fails on windows

    C:\>test.exe -test.v -test.run=ALE
    === RUN TestALEX
    --- FAIL: TestALEX (0.03 seconds)
    net_test.go:289: WSARecv tcp 127.0.0.1:1444: An existing
    connection was forcibly closed by the remote host.
    FAIL

    But both PASS if CloseWrite added.

    Alex

    http://codereview.appspot.com/6826084/
  • Brad Fitzpatrick at Nov 12, 2012 at 11:24 pm
    Cool, thanks for testing! Good to hear.
    On Mon, Nov 12, 2012 at 3:23 PM, wrote:
    On 2012/11/12 16:16:00, bradfitz wrote:

    On Sun, Nov 11, 2012 at 10:35 PM, <mailto:alex.brainman@gmail.**com<alex.brainman@gmail.com>
    wrote:
    I do not think it will help any.

    I think it will: ...
    You are right. I am wrong. It does help here. This test:

    diff --git a/src/pkg/net/net_test.go b/src/pkg/net/net_test.go
    --- a/src/pkg/net/net_test.go
    +++ b/src/pkg/net/net_test.go
    @@ -213,3 +213,85 @@
    t.Fatal(err)
    }
    }
    +
    +func TestALEX(t *testing.T) {
    + l, err := Listen("tcp", "127.0.0.1:0")
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer l.Close()
    +
    + read := func(r io.Reader) error {
    + var m [1]byte
    + _, err := r.Read(m[:])
    + return err
    + }
    +
    + write := func(w io.Writer) error {
    + var m [1]byte
    + _, err := w.Write(m[:])
    + return err
    + }
    +
    + ec := make(chan error)
    +
    + // server
    + go func() (err error) {
    + defer func() {
    + ec <- err
    + }()
    +
    + c, err := l.Accept()
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer c.Close()
    +
    + err = read(c)
    + if err != nil {
    + return err
    + }
    + err = write(c)
    + if err != nil {
    + return err
    + }
    +
    +// err = c.(*TCPConn).CloseWrite()
    + if err != nil {
    + return err
    + }
    + return nil
    + }()
    +
    + // client
    + c, err := Dial("tcp", l.Addr().String())
    + if err != nil {
    + t.Fatal(err)
    + }
    + defer c.Close()
    +
    + go func() {
    + var m [1000]byte
    + for {
    + _, err := c.Write(m[:])
    + if err != nil {
    + return
    + }
    + }
    + }()
    +
    + for {
    + err := read(c)
    + if err != nil {
    + if err == io.EOF {
    + break
    + }
    + t.Fatal(err)
    + }
    + }
    +
    + err = <-ec
    + if err != nil {
    + t.Fatal(err)
    + }
    +}

    fails on linux

    # go test -v -run=ALEX
    === RUN TestALEX
    --- FAIL: TestALEX (0.00 seconds)
    net_test.go:289: read tcp 127.0.0.1:38968: connection reset by
    peer
    FAIL
    exit status 1
    FAIL net 0.009s

    and fails on windows

    C:\>test.exe -test.v -test.run=ALE
    === RUN TestALEX
    --- FAIL: TestALEX (0.03 seconds)
    net_test.go:289: WSARecv tcp 127.0.0.1:1444: An existing
    connection was forcibly closed by the remote host.
    FAIL

    But both PASS if CloseWrite added.

    Alex

    http://codereview.appspot.com/**6826084/<http://codereview.appspot.com/6826084/>

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 12, '12 at 6:11a
activeNov 12, '12 at 11:51p
posts11
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase