Make bRun atomic

[ann0see]

This PR applies 1/1 suggestions from code quality AI findings.

It's very likely that we have various places where we have bugs due to assuming bools are atomic while they are not.

[ann0see]

CC: @softins - I'll need to try it locally but I'm fairly confident that we have a project wise misunderstanding about variables being thread safe while they are not.

[softins]

This doesn't look at all necessary to me. It's not like a counter being incremented, or some other read-modify-write operation. bRun is initialised to true, and then the loop just keeps running until it becomes false. Setting it to false is a single write, which by definition is atomic. I don't see any thread safety issue here at all.

[mcfnord]

https://stackoverflow.com/questions/16320838/when-do-i-really-need-to-use-atomicbool-instead-of-bool

It looks like each thread has its own bool that it reads and modifies. If so, I agree there's no risk of cached write by another thread here.

[ann0see]

Setting it to false is a single write, which by definition is atomic.

I am unsure if that's really the case. I remember that on x86 there are combinations that this doesn't hold: write to L1 of core A but core B still has the old value in its L1 cache. B may not get updated. This could yield to infinite looping.

Citing the stackoverflow post:

Without atomic there is no guarantee that you'll ever see the update in the other thread at all, or that you'll see updates to variables in the same order that you make them in a different thread.

Assuming the bool is really thread local it's fine - however it still stands that in the codebase we do have other areas where this is probably not true. I remember seeing a comment about the write being atomic on a bool where I questioned if this is really the case.

[softins]

Setting it to false is a single write, which by definition is atomic.

I am unsure if that's really the case. I remember that on x86 there are combinations that this doesn't hold: write to L1 of core A but core B still has the old value in its L1 cache. B may not get updated. This could yield to infinite looping.

Hmm, if that's really the case, I'd class that as a hardware bug! A write to a memory location ought to invalidate any cached copies of it. Unless there are subtleties that I have yet to understand, which is quite possible!

Citing the stackoverflow post:

Without atomic there is no guarantee that you'll ever see the update in the other thread at all, or that you'll see updates to variables in the same order that you make them in a different thread.

OK, so my next question to understand this would be: what, at the code level, does atomic do that overcomes this, that a standard write does not?

Assuming the bool is really thread local it's fine - however it still stands that in the codebase we do have other areas where this is probably not true. I remember seeing a comment about the write being atomic on a bool where I questioned if this is really the case.

[ann0see]

what, at the code level, does atomic do that overcomes this, that a standard write does not?

I believe it's a memory fence.

[pljones]

what, at the code level, does atomic do that overcomes this, that a standard write does not?

I believe it's a memory fence.

So there's a "memory fence" code operation?

[ann0see]

Maybe it's some assembly emitted by the compiler? https://stackoverflow.com/questions/27595595/when-are-x86-lfence-sfence-and-mfence-instructions-required

I

[ann0see]

I suspect the hang's root cause is stuff like this.

[softins]

Ok, I'm learning things about this that I didn't know before.

In this particular case, the loop testing bRun is in the separate high-priority thread that reads the inbound data from the socket(s). But the Stop() method that sets bRun to terminate the loop is likely called in the main application event thread.

So that suggests std::atomic<bool> would indeed be needed, which is new knowledge to me.

I do wonder if the explicit load and store methods are actually required? Are there not operator overloads within std::atomic to allow normal read and write syntax?

[ann0see]

Likely not needed to have explicit loads/stores, agree.

https://stackoverflow.com/questions/31978324/what-exactly-is-stdatomic

[softins]

According to Gemini, the explicit load/store allows you to specify the ordering and fencing for the operations, e.g. relaxed, but the overloaded operators will default to the strictest, for safety. For the modest rate at which we are testing in the loop, there is negligible penalty in just using the overloaded operators, and a benefit in simplicity.