code review 149340044: net/http: don't reuse a server connection after any Wri...

net/http: don't reuse a server connection after any Write errors

Fixes Issue 8534

[bradfitz, 2014-10-15 14:25:44]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

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

[adg, 2014-10-15 15:10:56]

https://codereview.appspot.com/149340044/diff/100001/src/net/http/serve_test.go
File src/net/http/serve_test.go (right):

https://codereview.appspot.com/149340044/diff/100001/src/net/http/serve_test....
src/net/http/serve_test.go:2664: // An similar test crashed once during
development, but it was only
s/An/A/

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go
File src/net/http/server.go (right):

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go#n...
src/net/http/server.go:117: w          io.Writer            //
checkConnErrorWriter's copy of wrc, not zeroed on Hijack
why don't we just put the writer in a field on checkConnErrWriter ? avoiding
allocs?

[bradfitz, 2014-10-15 15:24:03]

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go
File src/net/http/server.go (right):

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go#n...
src/net/http/server.go:117: w          io.Writer            //
checkConnErrorWriter's copy of wrc, not zeroed on Hijack
On 2014/10/15 15:10:55, adg wrote:
> why don't we just put the writer in a field on checkConnErrWriter ? avoiding
> allocs?

yes

[adg, 2014-10-15 15:27:52]

LGTM

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go
File src/net/http/server.go (right):

https://codereview.appspot.com/149340044/diff/100001/src/net/http/server.go#n...
src/net/http/server.go:117: w          io.Writer            //
checkConnErrorWriter's copy of wrc, not zeroed on Hijack
On 2014/10/15 15:24:03, bradfitz wrote:
> On 2014/10/15 15:10:55, adg wrote:
> > why don't we just put the writer in a field on checkConnErrWriter ? avoiding
> > allocs?
> 
> yes

well add a comment saying so

[bradfitz, 2014-10-15 15:50:59]

*** Submitted as https://code.google.com/p/go/source/detail?r=3266b7e742ba ***

net/http: don't reuse a server connection after any Write errors

Fixes Issue 8534

LGTM=adg
R=adg
CC=golang-codereviews
https://codereview.appspot.com/149340044

[gobot, 2014-10-16 03:34:54]

This changed caused perf changes on windows-amd64-perf:


json-1                    old          new      delta
cputime             181562500    190000000      +4.65
time                181563710    190151452      +4.73

json-2                    old          new      delta
cputime             183125000    189375000      +3.41
time                 92173109     95976543      +4.13

json-4                    old          new      delta
cputime             185703125    192343750      +3.58
time                 47237594     48829030      +3.37

json-8                    old          new      delta
cputime             195875000    202593750      +3.43
time                 25044583     26039480      +3.97

http://build.golang.org/perfdetail?commit=3266b7e742ba13bf2207d7e144715565328...



—gobot