code review 6826084: net/http: handle 413 responses more robustly

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

[bradfitz, 2012-11-12 06:11:50]

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/

[brainman, 2012-11-12 06:35:44]

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

[bradfitz, 2012-11-12 16:16:00]

On Sun, Nov 11, 2012 at 10:35 PM, <alex.brainman@gmail.com> 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.

[rsc, 2012-11-12 20:54:03]

LGTM

What a mess.

[bradfitz, 2012-11-12 23:20:21]

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

[brainman, 2012-11-12 23:23:53]

On 2012/11/12 16:16:00, bradfitz wrote:
> On Sun, Nov 11, 2012 at 10:35 PM, <mailto: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

[bradfitz, 2012-11-12 23:24:44]

Cool, thanks for testing! Good to hear.

On Mon, Nov 12, 2012 at 3:23 PM, <alex.brainman@gmail.com> 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/>
>