Race condition between net/http.(*response).Write() and Flush()

[Fumin Wang]

Hi

I wonder are net/http.(*response).Write() and net/http.(*response).Flush() expected to be called by only one goroutine? After much searching, the closest discussion I could find was https://groups.google.com/forum/#!searchin/golang-nuts/race$20http/golang-nuts/Pp_5EvbymEM/Xy-p96Rr4joJ , but I still couldn't fully answer my doubts.

My main intention is to implement Server sent events http://www.html5rocks.com/en/tutorials/eventsource/basics/ in my web app. To detect client disconnects, I'm having a separate goroutine besides the one running my http.HandleFunc() to send heartbeats https://github.com/fumin/webutil/blob/master/sse.go#L55 . However, by having two goroutines calling Write() and Flush() at the same time, the Go race detector reports:

==================

WARNING: DATA RACE

Write by goroutine 22:

  runtime.copy()

      /usr/local/go/src/pkg/runtime/slice.c:120 +0x0

  bufio.(*Writer).Write()

      /usr/local/go/src/pkg/bufio/bufio.go:538 +0x40a

  net/http.(*response).write()

      /usr/local/go/src/pkg/net/http/server.go:986 +0x32c

  net/http.(*response).Write()

      /usr/local/go/src/pkg/net/http/server.go:958 +0x88

  ...

  github.com/fumin/webutil.func·001()

      /var/lib/openshift/key/app-root/data/build/src/github.com/fumin/webutil/sse.go:55 +0x160

Previous read by goroutine 21:

  runtime.copy()

      /usr/local/go/src/pkg/runtime/slice.c:120 +0x0

  bufio.(*Writer).Write()

      /usr/local/go/src/pkg/bufio/bufio.go:538 +0x40a

  net/http.(*chunkWriter).Write()

      /usr/local/go/src/pkg/net/http/server.go:259 +0x3e5

  bufio.(*Writer).flush()

      /usr/local/go/src/pkg/bufio/bufio.go:494 +0x15a

  bufio.(*Writer).Flush()

      /usr/local/go/src/pkg/bufio/bufio.go:483 +0x34

  net/http.(*response).Flush()

      /usr/local/go/src/pkg/net/http/server.go:1023 +0x7f

I wonder is it a reasonable requirement for a http.(*response) to support concurrent writes? If not, am I supposed to implement additional synchronizations using something like a sync.Mutex? If so, where should I add the mutex, perhaps in the form of:

// ConcurrentResponseWriter is too verbose, is there a better name for it?

type ConcurrentResponseWriter struct {

  sync.Mutex

  *http.ResponseWriter

}

func (w * ConcurrentResponseWriter) Write(b []byte) error {

  w.Lock()

  defer w.Unlock()

  return w.Write(b)

}

func (w * ConcurrentResponseWriter) Flush() {

  w.Lock()

  defer w.Unlock()

  w.Flush()

}

looks a bit too verbose, and too much trouble.

[Kevin Malachowski]

As far as I know it isn't safe to use the writer concurrently. What should happen if two writes happen at the same time? Which should go first? What happens if the writes are too big and have to get split into multiple packets? I doubt that interleaving these smaller packets would be what you want.

Sticking to one goroutine per connection is the easiest way to handle connections in my experience. You could use a select along with a time.Ticker (or something like that) in a loop in your handle function to implement a heartbeat.

[James Bardin]

I wonder is it a reasonable requirement for a http.(*response) to support concurrent writes? If not, am I supposed to implement additional synchronizations using something like a sync.Mutex? If so, where should I add the mutex, perhaps in the form of:

// ConcurrentResponseWriter is too verbose, is there a better name for it?

Why? Keystrokes don't cost much.

 

type ConcurrentResponseWriter struct { 

  sync.Mutex

  *http.ResponseWriter

}

func (w * ConcurrentResponseWriter) Write(b []byte) error {

  w.Lock()

  defer w.Unlock()

  return w.Write(b)

}

func (w * ConcurrentResponseWriter) Flush() {

  w.Lock()

  defer w.Unlock()

  w.Flush()

}

looks a bit too verbose, and too much trouble.

How is this too verbose, or a "too much trouble"?  It easily gives you protected access to Write and Flush, and is generally how you wrap and delegate methods for an embedded type.

[James Bardin]

On Tuesday, May 13, 2014 10:54:48 AM UTC-4, James Bardin wrote:

func (w * ConcurrentResponseWriter) Write(b []byte) error {

  w.Lock()

  defer w.Unlock()

  return w.Write(b)

}

Oops, but this is recursive, you want `return w.ResponseWriter.Write(b)`

 

[Kevin Malachowski]

This is sort of what I mean: http://play.golang.org/p/2SLUwON3_T

 The Connection.data channel idea would work well if you have data that needs to be sent to clients on other goroutines. Another way to do it is to generate the data in each client's goroutine, but which is best depends on your specific use case.

[Fumin Wang]

Yes, I'm all aware we can circumvent this by having the time.Ticker in the http.HandleFunc goroutine. 

What I want to ask is what is the solution to the general problem of implementing heartbeats. Is it not possible to hide away logic related to the heartbeats, or is it mandatory for heartbeat logic to stay inside the main goroutine handling the network connection.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/oYwNE68P68M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Matt Harden]

I would have one goroutine write to the connection, perhaps by looping on an input channel then formatting and writing out the data from the channel. A separate goroutine could send heartbeats on the channel using time.Ticker. I would stop the ticker when the connection is closed.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.