fix racey boolean checks #25
Conversation
There was a problem hiding this comment.
I assume this is motivated by go race detector saying these assignments and reads were racey before.
The one issue I see here is that it doesn't fix any logical races that might exist here - taking an action based on the value and then changing it can still race because there is no atomic check-and-set being used.
These nitpicks may well not affect this code at all in practice, but I'm wary of using atomics and "lock free" programming techniques without very careful attention and documentation to the invariants that are and aren't upheld.
It might be easier to refactor the lifecycle stuff to occur all in one goroutine and message in and out with channels or to use actual mutexes around whole critical sections containing boolean flag test and subsequent actions. Even if these are safe with current usage, reasoning about that is very hard and it doesn't seem to be a performance-critical path to warrant the complexity of correctly using advanced concurrency techniques.
I also may have totally missed the context and be way off, but I'm happy to talk about it and figure out if I'm being too cautios... (or plain wrong in my assumptions).
| rn.started = false | ||
| rn.initialized = false | ||
| rn.started.Unset() | ||
| rn.initialized.Unset() |
There was a problem hiding this comment.
Do you rely on the ordering of these atomic bool changes? Because ordering like this on two different memory locations is not guaranteed.
Golang's memory model in the spec says:
That is, compilers and processors may reorder the reads and writes executed within a single goroutine only when the reordering does not change the behavior within that goroutine as defined by the language specification. Because of this reordering, the execution order observed by one goroutine may differ from the order perceived by another.
So you can't rely on the order these are set being the same between two separate goroutines running the same code potentially.
This may well not matter, but if it breaks assumptions to re-order these randomly then it's still racey.
There was a problem hiding this comment.
In this case, no, the orderings don't matter. Just that they are both unset
| @@ -369,12 +369,12 @@ func (rn *Node) Destroy() error { | |||
| } | |||
| rn.logger.Debug("Successfully removed self from canoe cluster") | |||
|
|
|||
| if rn.running { | |||
| if rn.running.IsSet() { | |||
There was a problem hiding this comment.
If two goroutines can be running this it's still racey since they might both observe it being set and run this section.
Even if you are sure this can only be run by a single goroutine, it's still potentially racey because other goroutines might still get a response to node.isHealthy() of true even after the stop chan is closed and transport is partially shutdown. That might not cause actual problems in practice but it probably violates assumptions about what is safe to assume and what isn't since none are documented in the methods.
There was a problem hiding this comment.
Hrm. Yeah you're right. I'll look back over and see about using mutexes possibly - or some other construct. Or a check and set possibly
No description provided.