search

SlyMarbo / spdy

[deprecated] A full-featured SPDY library for the Go language.
BSD 2-Clause "Simplified" License
116 stars 13 forks source link

race condition in Header/Close #31

Closed cpg closed 11 years ago

cpg commented 11 years ago

here is an interesting race condition reported.

WARNING: DATA RACE
Read by goroutine 14:
  github.com/SlyMarbo/spdy.(*serverStreamV3).Header()
      /home/cpg/proxy/src/github.com/SlyMarbo/spdy/spdy3_server_stream.go:43 +0x38
  main.(*responseCopier).ReceiveHeader()
      /home/cpg/proxy/src/proxy/service.go:40 +0x5c
...
Previous write by goroutine 19:
  github.com/SlyMarbo/spdy.(*serverStreamV3).Close()
      /home/cpg/proxy/src/github.com/SlyMarbo/spdy/spdy3_server_stream.go:147 +0x1fe
  github.com/SlyMarbo/spdy.(*connV3).handleRstStream()
      /home/cpg/proxy/src/github.com/SlyMarbo/spdy/spdy3_conn.go:713 +0x4d0
...
SlyMarbo commented 11 years ago

Although this is, arguably, a race condition, it does not perform unsafely. At this point the connection has been closed, so although it is uncertain whether Header() will return the connection's former header or nil, either is acceptable.

cpg commented 11 years ago

yes. i don't understand why it's even reported as a race, given that there is an s.Lock at the top and s.Unlock deferred.

SlyMarbo commented 11 years ago

The reason it reports a race is that Header() doesn't use a lock, so it can still happen at the same time as the write, but in this instance that is still safe.