Is there such thing as "benign race condition"?

[leventov]

@peterbourgon to avoid edit wars, I invite you to have discussions here.

Many people use the term "benign race condition", including in literature.

The benign race condition I referred to in the checklist would be like this:

var m sync.Map

func foo(k string) string {
	v, ok := m.Load(k)
	if !ok {
		v = compute(k)
		m.Store(k, v)
	}
	return v.(string)
}

Assuming compute() is safe for concurrent calling and idempotent.

[peterbourgon]

"Data race" is unfortunately not a well defined term. Broadly, there are two classes of data races. One class is totally safe, but somewhat reliably produces nondeterministic behavior. Another class is totally unsafe, but almost never manifests itself in practice.

Your example demonstrates a class-1 race, and I agree by that definition the race is benign. But when we use the term "benign" we are explicitly talking about memory safety, which only class-2 races violate. So the problem here is that you're creating confusion — the phrase "benign race condition" may be true if we mean class-1 races, but that's not the obvious interpretation, and in fact is frequently used to excuse class-2 data races which are absolutely not benign! As an example, here is a PR on some project which fixes a class-2 data race that the maintainer insists is benign. (The maintainer is incorrect.)

So like

func DoSomething(k Key, v Value) {
	existing, ok := m.Load(k)
	if !ok {
		m.Store(k, v) // RACE CONDITION - two goroutines can execute this in parallel

This is misleading. It's a class-1 data race, but not a class-2 race condition. There's nothing unsafe about it. An advisory document needs to be more precise, at a minimum.

[leventov]

I don't think your terminology matches the accepted in the industry.

We started started with "benign races conditions", not data races.

Data race = reads/writes from/to memory without synchronisation and/or memory barriers.
Race condition = generally speaking, "concurrently happening stuff", even if completely covered by synchronisation and barriers (e. g. all variables being updated are atomic).

Wikipedia classifies data races as a subset of race conditions, which is a slightly different view, but generally compatible.

There are benign race conditions. There are even benign data races, but this is something very obscure (and there, the term "benign" becomes oxymoron -- it's far too easy to make a mistake). Java's String.hashCode() is an infamous example.

So the problem here is that you're creating confusion — the phrase "benign race condition" may be true if we mean class-1 races, but that's not the obvious interpretation, and in fact is frequently used to excuse class-2 data races which are absolutely not benign.

Probably you are right. But I think having a point with some recommendation and a badge "false information" beneath it only increases the confusion even more. If the "data race/race condition" terminology is not clear to the majority of people (which is probably true), let's try to clear it out, and give examples, and add NBs. To come up with a consistent piece of text that would be useful for the majority of people.

[peterbourgon]

Your distinction is noted and understood, but it not well-understood by the industry.

If the "data race/race condition" terminology is not clear to the majority of people (which is probably true), let's try to clear it out, and give examples, and add NBs. To come up with a consistent piece of text that would be useful for the majority of people.

Sure. I think everything is solved if you simply remove the terms "race condition" and "benign" from your example altogether. For example

There is no guarantee that the value read by the Load instruction on line X will still be there when we execute our Store instruction on line Y. Another goroutine could have Stored a different value while we were processing the data in compute. If you need to guarantee that no change occurs between Load and Store, you must use an additional layer of synchronization, for example a sync.Mutex, to guard the entire Load/compute/Store block.

[peterbourgon]

In fact, dropping the phrase "race condition" from the document entirely would be ideal.

[peterbourgon]

Also, I apologize for the way the comment in the wiki page was phrased. It was imprecise and, as you suggest, misleading in its own way. I've corrected it, even if only for a short while, if you update the text.