search

SlyMarbo / spdy

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

Fix race condition in spdy3_conn #46

Closed rnapier closed 10 years ago

rnapier commented 10 years ago

Discovered with the go race detector. It races with processFrame() around line 1121.

SlyMarbo commented 10 years ago

I'm not 100% convinced, but I'm pretty sure this is a false positive. Since the method simply returns the field without modifying it, it's essentially atomic already. Adding a lock wouldn't really make any difference apart from the extra overhead and the risk of a deadlock (if the method is called when the lock has already been taken).

As a result, I'm going to refuse this, but feel free to make another pull if there's more concrete evidence that it's actually a race condition with side effects.

rnapier commented 10 years ago

It actually does create a deadlock.

rnapier commented 10 years ago

Note that this makes it very challenging for dependent projects to use the race detector on their own code since there's no way to exclude your project. It's not a false positive; it's a race that happens during actual execution; it just may or may not have an obvious symptom. There is no promise that a 32-bit read/write is atomic on all supported platforms. To a similar question, minux's answer was "no guarantees from the Go memory model... this will be fine in practice, but again, not guaranteed. You'd better use a mutex." https://groups.google.com/d/msg/golang-nuts/8G9-vp5DwpU/jP1iWeC_Ph0J Writing a uint32 to an unaligned variable is explicitly not atomic on ARM (meaning you can get a garbage value in a racing reader). The current Go compiler does seem to use aligned variables, but it does not promise that, so it's undefined behavior.

It would be very helpful if spdy would clear the race detector without errors. (It currently spits out about 20 spdy-related races in my unit tests, which is preventing me from adding the race detector to my builds.)

SlyMarbo commented 10 years ago

I stand corrected.

My main concern was that there might be a situation where the method gets called when the lock has already been acquired. This doesn't look likely, so I'll merge.

rnapier commented 10 years ago

Actually, I believe that does happen. In my later tests, it did seem to be deadlocking. So this isn't actually the right fix (I should have tested it more extensively before submitting). So something is needed, but this is too simple.​