Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(41)

Issue 6826084: code review 6826084: net/http: handle 413 responses more robustly (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by bradfitz
Modified:
9 years, 7 months ago
Reviewers:
brainman
CC:
golang-dev, rsc
Visibility:
Public.

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

Patch Set 1 #

Patch Set 2 : diff -r 422c00e8e022 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 422c00e8e022 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 723ce500dd2c https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -18 lines) Patch
M src/pkg/net/http/server.go View 1 2 3 3 chunks +22 lines, -18 lines 0 comments Download

Messages

Total messages: 7
bradfitz
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/
9 years, 7 months ago (2012-11-12 06:11:50 UTC) #1
brainman
I do not think it will help any. But, I can't see it hurt either. ...
9 years, 7 months ago (2012-11-12 06:35:44 UTC) #2
bradfitz
On Sun, Nov 11, 2012 at 10:35 PM, <alex.brainman@gmail.com> wrote: > I do not think ...
9 years, 7 months ago (2012-11-12 16:16:00 UTC) #3
rsc
LGTM What a mess.
9 years, 7 months ago (2012-11-12 20:54:03 UTC) #4
bradfitz
*** Submitted as http://code.google.com/p/go/source/detail?r=e6b3e4d247ef *** net/http: handle 413 responses more robustly When HTTP bodies were ...
9 years, 7 months ago (2012-11-12 23:20:21 UTC) #5
brainman
On 2012/11/12 16:16:00, bradfitz wrote: > On Sun, Nov 11, 2012 at 10:35 PM, <mailto:alex.brainman@gmail.com> ...
9 years, 7 months ago (2012-11-12 23:23:53 UTC) #6
bradfitz
9 years, 7 months ago (2012-11-12 23:24:44 UTC) #7
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/>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b