Bool is not atomic

[timou]

The Go race detector doesn't like concurrent access to bool. This change
secures the bool with a mutex.

[msftclas]

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

[darstahl]

We already have an "atomicBool" type in this file used for a different bool with a similar issue. I'd prefer to use that instead of adding a Mutex.

type atomicBool int32

func (b *atomicBool) isSet() bool { return atomic.LoadInt32((*int32)(b)) != 0 }
func (b *atomicBool) setFalse()   { atomic.StoreInt32((*int32)(b), 0) }
func (b *atomicBool) setTrue()    { atomic.StoreInt32((*int32)(b), 1) }

See deadlineHandler.timedout for example usage.

[timou]

That's fine, and I'm happy to change that, but I also think there is a genuine race between inspecting the bool and setting it (lines 112-114 of file.go). I don't think that can be solved by atomicbool.

[timou]

OK, I lie. I have extended atomicBool with compareAndSwap, which I think fixes the race and avoids the mutex.

[darstahl]

This can be all one line.

[darstahl]

@timou I was about to comment almost the same fix.

While compareAndSwap works, as per an offline conversation with @jstarks only a swap is needed here (and is faster).

func (b *atomicBool) swap(new bool) (old bool) {
	var newInt int32
	if new {
		newInt = 1
	}
	return atomic.SwapInt32((*int32)(b), newInt) == 1
}

[darstahl]

Could you also squash your commits into a single commit before we merge this?

[darstahl]

LGTM

[darstahl]

There's still a second (blank) merge commit. I'm good to merge if you can remove that.

[timou]

OK, rebased. Thanks for the nice review!