Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: TimeoutHandler header race on timeout #9162

Closed
wongak opened this issue Nov 25, 2014 · 8 comments

Comments

Projects
None yet
7 participants
@wongak
Copy link

commented Nov 25, 2014

Hi, this might be related to 

https://golang.org/issue/8414
and
https://golang.org/issue/8209

What does 'go version' print?

$ ./go version
go version devel +59dbd878547f Tue Nov 25 08:42:00 2014 +0100 linux/amd64

What steps reproduce the problem?

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

modifying a header before timing out.

What happened?

$ go test -race

produces a race warning.

What should have happened instead?

No data race should occur.

Please provide any additional information below.

After digging a little bit, it seems like the way the cloning is set up prevents
handlers to be run in another goroutine other than the one created by http.Server.

WARNING: DATA RACE
Read by goroutine 20:
  net/http.(*response).Header()
      /home/user/gotip/src/net/http/server.go:609 +0x62
  net/http.(*timeoutWriter).Header()
      /home/user/gotip/src/net/http/server.go:1944 +0x5b
  net/http_test.func·073()
      /home/user/gotip/src/net/http/serve_test.go:1174 +0x4d
  net/http.HandlerFunc.ServeHTTP()
      /home/user/gotip/src/net/http/server.go:1265 +0x4e
  net/http.func·018()
      /home/user/gotip/src/net/http/server.go:1918 +0xfe

Previous write by goroutine 17:
  net/http.(*response).WriteHeader()
      /home/user/gotip/src/net/http/server.go:639 +0x17d
  net/http.(*timeoutHandler).ServeHTTP()
      /home/user/gotip/src/net/http/server.go:1928 +0x4c3
  net/http/httptest.(*waitGroupHandler).ServeHTTP()
      /home/user/gotip/src/net/http/httptest/server.go:200 +0xf7
  net/http.serverHandler.ServeHTTP()
      /home/user/gotip/src/net/http/server.go:1703 +0x1f6
  net/http.(*conn).serve()
      /home/user/gotip/src/net/http/server.go:1204 +0x1087

Goroutine 20 (running) created at:
  net/http.(*timeoutHandler).ServeHTTP()
      /home/user/gotip/src/net/http/server.go:1920 +0x2ca
  net/http/httptest.(*waitGroupHandler).ServeHTTP()
      /home/user/gotip/src/net/http/httptest/server.go:200 +0xf7
  net/http.serverHandler.ServeHTTP()
      /home/user/gotip/src/net/http/server.go:1703 +0x1f6
  net/http.(*conn).serve()
      /home/user/gotip/src/net/http/server.go:1204 +0x1087

Goroutine 17 (running) created at:
  net/http.(*Server).Serve()
      /home/user/gotip/src/net/http/server.go:1751 +0x3cd
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2014

Comment 1:

Labels changed: added repo-main, release-go1.5.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Nov 26, 2014

Comment 2:

Labels changed: added threadsanitizer.

@wongak

This comment has been minimized.

Copy link
Author

commented Nov 26, 2014

Comment 3:

Could this be solved by adding a mutex?
diff -r 59dbd878547f src/net/http/server.go
--- a/src/net/http/server.go    Tue Nov 25 08:42:00 2014 +0100
+++ b/src/net/http/server.go    Wed Nov 26 10:44:33 2014 +0100
@@ -306,6 +306,7 @@
    cw chunkWriter
    sw *switchWriter // of the bufio.Writer, for return to putBufioWriter
 
+   hmu sync.Mutex
    // handlerHeader is the Header that Handlers get access to,
    // which may be retained and mutated even after WriteHeader.
    // handlerHeader is copied into cw.header at WriteHeader
@@ -606,6 +607,8 @@
 }
 
 func (w *response) Header() Header {
+   w.hmu.Lock()
+   defer w.hmu.Unlock()
    if w.cw.header == nil && w.wroteHeader && !w.cw.wroteHeader {
        // Accessing the header between logically writing it
        // and physically writing it means we need to allocate
@@ -640,7 +643,9 @@
    w.status = code
 
    if w.calledHeader && w.cw.header == nil {
+       w.hmu.Lock()
        w.cw.header = w.handlerHeader.clone()
+       w.hmu.Unlock()
    }
 
    if cl := w.handlerHeader.get("Content-Length"); cl != "" {

@wongak wongak added new labels Nov 26, 2014

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014

@rsc rsc removed the repo-main label Apr 14, 2015

@bradfitz bradfitz changed the title net/http: ResponseWriter.Header() prevents handlers being run in separate goroutines net/http: TimeoutHandler header race on timeout Apr 28, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

A mutex doesn't help here. The problem is that the TimeoutHandler executes the handler in a separate goroutine, which may obtain the headers via ResponseWriter.Header() and the TimeoutHandler's main ServeHTTP, upon timeout, may also access ResponseWriter.Header(). Even with a mutex there guarding the acquisition of the map, they both may subsequently race on accessing the header, for which there's no place to put a mutex.

I think the solution is to change the design of the Timeout Handler to instead Hijack the connection if possible and write the timeout response headers & body by hand, if possible. There are still problems with that, though: requires the ResponseWriter to be a Hijacker, and assumes HTTP/1.n (but this isn't the first such problem), and prevents wrapper Handlers from intercepting the error and writing a prettier failure HTML body.

So I think I'll change things to avoid this in the common case but I think the bigger problem is that TimeoutHandler isn't compatible with the ResponseWriter & Header design. We probably should've propagated a Context type (like https://godoc.org/golang.org/x/net/context#Context) through ServeHTTP instead, to which we could attach deadlines and do cancelation. Alas.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Go1.6, Go1.5, Go1.6Early Jun 29, 2015

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2015

Ping @bradfitz . Is anything here going to happen for 1.6?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.6, Go1.6Early Dec 11, 2015

@extemporalgenome

This comment has been minimized.

Copy link

commented Dec 11, 2015

Can timeoutWriter store a separate "inner" header map specifically for use by the wrapped Handler? When WriteHeader (or the first Write) occurs, if timedOut is false, the inner map can be key-wise copied (with mutex protection) into the original ResponseWriter's map. In any case, the two maps remain isolated.

Some special handling would need to occur for trailers to function properly.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 14, 2015

CL https://golang.org/cl/17752 mentions this issue.

@bradfitz bradfitz closed this in 0478f7b Dec 14, 2015

@golang golang locked and limited conversation to collaborators Dec 14, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.