code review 123610043: net/http: fix TimeoutHandler data races; hold lock longer

net/http: fix TimeoutHandler data races; hold lock longer

The existing lock needed to be held longer. If a timeout occured
while writing (but after the guarded timeout check), the writes
would clobber a future connection's buffer.

Also remove a harmless warning by making Write also set the
flag that headers were sent (implicitly), so we don't try to
write headers later (a no-op + warning) on timeout after we've
started writing.

Fixes Issue 8414
Fixes Issue 8209

[bradfitz, 2014-08-19 19:33:07]

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

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

[ruiu, 2014-08-19 19:46:49]

Is WriteHeader safe? If a timeout occurred while WriteHeader
is writing, would it clobber the buffer?

[bradfitz, 2014-08-19 19:51:35]

Good point.
 On Aug 19, 2014 12:46 PM, <ruiu@google.com> wrote:

> Is WriteHeader safe? If a timeout occurred while WriteHeader
> is writing, would it clobber the buffer?
>
> https://codereview.appspot.com/123610043/
>

[bradfitz, 2014-08-19 20:22:15]

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

Please take another look.

[bradfitz, 2014-08-19 20:22:28]

PTAL. Fixed WriteHeader too and added another test.

Both tests previously failed and work now.



On Tue, Aug 19, 2014 at 12:51 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> Good point.
>  On Aug 19, 2014 12:46 PM, <ruiu@google.com> wrote:
>
>> Is WriteHeader safe? If a timeout occurred while WriteHeader
>> is writing, would it clobber the buffer?
>>
>> https://codereview.appspot.com/123610043/
>>
>

[ruiu, 2014-08-19 20:28:17]

LGTM with this fix.

https://codereview.appspot.com/123610043/diff/60001/src/pkg/net/http/serve_te...
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/123610043/diff/60001/src/pkg/net/http/serve_te...
src/pkg/net/http/serve_test.go:1263: res.Body.Close()
res.Body.Close() called twice at this line and two lines above.

[adg, 2014-08-20 00:58:27]

typo in CL description "warming"

[adg, 2014-08-20 01:02:43]

LGTM after ruiu comments

[bradfitz, 2014-08-20 01:45:11]

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

net/http: fix TimeoutHandler data races; hold lock longer

The existing lock needed to be held longer. If a timeout occured
while writing (but after the guarded timeout check), the writes
would clobber a future connection's buffer.

Also remove a harmless warning by making Write also set the
flag that headers were sent (implicitly), so we don't try to
write headers later (a no-op + warning) on timeout after we've
started writing.

Fixes Issue 8414
Fixes Issue 8209

LGTM=ruiu, adg
R=adg, ruiu
CC=golang-codereviews
https://codereview.appspot.com/123610043

[seong, 2014-11-24 14:52:26]

Created a slight modification to the test:

diff -r 493ad916c3b1 src/net/http/serve_test.go
--- a/src/net/http/serve_test.go	Sun Nov 23 15:13:48 2014 -0500
+++ b/src/net/http/serve_test.go	Mon Nov 24 15:50:11 2014 +0100
@@ -1171,6 +1171,7 @@
 	sendHi := make(chan bool, 1)
 	writeErrors := make(chan error, 1)
 	sayHi := HandlerFunc(func(w ResponseWriter, r *Request) {
+		w.Header().Set("Content-Type", "text/plain")
 		<-sendHi
 		_, werr := w.Write([]byte("hi"))
 		writeErrors <- werr

Assuming a handler modifies the header before timing out. This will lead to a
data race.