Audit: non-atomic booleans and ints shared across threads (follow-up to #3786)

[mcfnord]

Background. In #3786 the discussion converged on two points: CSocketThread::bRun is written by one thread and read by another, so it should be std::atomic<bool>; and the same pattern probably exists elsewhere in the codebase. I audited main (55e0d62) file by file, tracing each shared flag against the threads that actually touch it. Below is everything I found, with file/line references only (no patch — that can follow separately if there's interest).

One data point up front: there are currently zero uses of std::atomic or QAtomic* anywhere in src/.

Why these matter, briefly. Each item is a data race in the C++ memory-model sense (concurrent access, at least one write, no synchronization), which is formally undefined behavior. In practice the realistic failure modes are: (a) the compiler legally caching or hoisting a repeated read (more likely under LTO/aggressive optimization), and (b) delayed or reordered visibility on weakly-ordered CPUs — which today includes Apple Silicon Macs, Android (Oboe), and ARM Linux servers such as Raspberry Pi. For the non-bool items, torn reads of multi-byte values are also possible. On x86 most of these "work" today, which is why they've been harmless-looking for years.

Thread model as audited.

• Client: main/Qt event thread (GUI, protocol — protocol messages arrive via queued signal, so parsing runs on main), socket thread (CSocketThread, time-critical), audio-driver callback thread (JACK/CoreAudio/ASIO/Oboe).
• Server: main thread (including OnTimer mixing, reached via queued signal from the timer thread, and the protocol slots which lock Mutex), CHighPrecisionTimer thread (non-Windows), socket thread, CThreadPool workers, recorder thread.

---

A. Thread-lifecycle bRun flags (the exact #3786 class)

A1. CSocketThread::bRun — src/socket.h:241.
Written false by the main thread in Stop() (socket.h:213); read in the thread loop (socket.h:231) on the socket thread. This is the #3786 case, unchanged on main.

A2. CHighPrecisionTimer::bRun (non-Windows) — src/util.h:1355.
Written by the main thread in Start()/Stop() (src/util.cpp:315, 339); read in the run() loop on the timer thread (src/util.cpp:348). Additionally read lock-free by a third thread: the socket thread calls pServer->IsRunning() (src/socket.cpp:744) → CServer::IsRunning() (src/server.h:136) → isActive() (src/util.h:1350) when deciding whether to wake a sleeping server. A missed bRun = false means the server's time-critical timer thread never exits its loop (Stop() then hits the 5 s wait() timeout with the thread still running). This affects every Linux/macOS server; the Windows variant is a QTimer on the main thread and is fine.

A3. CSoundBase::bRun — src/sound/soundbase.h:196.
Written by the main thread in Start() (soundbase.h:98) and Stop() (src/sound/soundbase.cpp:71); read on the realtime audio callback thread in every backend that checks it:

• src/sound/jack/sound.cpp:368 (read inside MutexAudioProcessCallback, but the writers don't hold that mutex, so the race remains),
• src/sound/coreaudio-mac/sound.cpp:1026 and :1086,
• src/sound/oboe/sound.cpp:225 (no lock at all at the point of the read).
  This flag is the only gate between the driver callback and the whole client audio processing chain.

A4. CSoundBase::bCallbackEntered — src/sound/soundbase.h:197.
Written true by the audio callback thread (soundbase.h:192), reset false by the main thread in Start() (soundbase.h:99), read by the GUI at src/clientdlg.cpp:1192 to detect a dead audio device. Unsynchronized write/reset/read across three call sites and two threads; a stale read here produces a false "sound card not working" verdict (or masks a real one).

B. Jitter-buffer status flags

B1. CSocket::bJitterBufferOK — src/socket.h:127.
Written false by the socket thread (src/socket.cpp:714); read-and-reset by the main thread in GetAndResetbJitterBufferOKFlag() (src/socket.cpp:519–526) with no lock — note the class has a Mutex (used in SendPacket, socket.cpp:443), it just isn't used here. Lost updates show a green jitter-buffer LED during dropouts.

B2. CClient::bJitterBufferOK — src/client.h:434.
Same pattern one level up: written false by the sound thread (src/client.cpp:1558), reset/read by the main thread (client.cpp:634, 641, 1084).

C. GUI-thread parameters read by the audio callback (client)

All written by plain setters on the main thread, read every frame by ProcessAudioDataIntern() on the sound thread, no atomics or locks:

Member	Declared	Written	Read (sound thread)
bMuteOutStream	src/client.h:393	client.h:281	client.cpp:1504, 1516, 1534, 1584
iInputBoost	src/client.h:409	client.h:290	client.cpp:1427–1433
iReverbLevel	src/client.h:407	client.h:192	client.cpp:1443–1445
bReverbOnLeftChan	src/client.h:406	client.h:197	client.cpp:1445
iAudioInFader	src/client.h:405	client.h:189	client.cpp:1449+

One of these is worse than a flag race: SetReverbOnLeftChan() (src/client.h:195–199) calls AudioReverb.Clear() on the GUI thread while the sound thread may be inside AudioReverb.Process() (client.cpp:1445). That is a concurrent mutation of non-trivial filter state, not just a stale boolean.

(For contrast: parameters whose setters stop/re-init/restart the sound — SetAudioChannels, SetAudioQuality, SetEnableOPUS64, SetSndCrdDev, OnRawAudioSupported, etc. — are structurally fine, though their "is the callback really stopped" handshake itself rides on the non-atomic bRun of A3.)

D. Channel state (client side)

D1. CChannel::bIsEnabled — src/channel.h:239.
Written under Mutex by SetEnable() (src/channel.cpp:152) on the main thread at connect/disconnect (client.cpp:1039, 1056); but read without the mutex via IsEnabled() (channel.h:106) by the socket thread in PutAudioData() (channel.cpp:561). A mutex only on the writer side synchronizes nothing. (Server side is safe by accident: channels are enabled once in the constructor, server.cpp:241, before any thread exists.)

D2. CChannel::iConTimeOut — the connection-liveness counter.
Disconnect() writes iConTimeOut = 1 with no lock (src/channel.cpp:536, main thread), while the socket thread resets it under MutexSocketBuf (channel.cpp:613 via channel.h:101) and the audio path decrements it under MutexSocketBuf (channel.cpp:635–641). IsConnected() (channel.h:102) reads it with no lock from several threads. A lost update here delays or cancels a disconnect — possibly related to the comment in CClient::Stop() that "disconnect is still not working reliably" (client.cpp:1065).

D3. CChannel::iNetwFrameSize / iNetwFrameSizeFact — mismatched mutexes.
Written under Mutex in SetAudioStreamProperties() (src/channel.cpp:201–224); read under MutexSocketBuf — a different mutex — by the socket thread in PutAudioData() (channel.cpp:566). Two different mutexes provide no mutual exclusion; the packet-size check can see a torn/stale value while transport properties change.

E. Server

E1. CServer::bChannelIsNowDisconnected — src/server.h:260.
Written by CThreadPool workers (src/server.cpp:950, enqueued at server.cpp:710) — potentially by several workers in the same tick. The read at server.cpp:722 is synchronized (it happens after future.wait()), but two workers storing concurrently is still formally a data race. This site carries the exact comment discussed in #3786 (server.cpp:947–949):

// note that no mutex is needed for this shared resource since it is not a
// read-modify-write operation but an atomic write …

A plain bool store is not an atomic write in the C++ memory model; making the member std::atomic<bool> would make the comment true at zero practical cost.

F. Shutdown path

F1. CSocket::UdpSocket4 / UdpSocket6 in Close() — src/socket.cpp:389+.
Close() (main thread) shuts the sockets and overwrites the fds with INVALID_SOCKET, with no lock, while the socket thread is concurrently using those same fds in the blocking poll/recvfrom loop of OnDataReceived(), and while SendPacket() (which does take Mutex, socket.cpp:443) may be sending from the sound thread. Close-while-in-use is the intended unblock mechanism, but the unsynchronized fd overwrite adds a stale-fd/fd-reuse hazard on top of the bRun race of A1.

---

Checked and found properly synchronized (for completeness)

• CServer::PutAudioData() and both protocol slots lock the server Mutex (server.cpp:1562, 1570, 1585), so the server's put/get audio path and channel protocol state are serialized with OnTimer.
• OnTimer holds Mutex for the whole decode phase and joins all workers via future.wait() before reading their results (server.cpp:667–719).
• CChannel::SetAudioStreamProperties() re-initializes SockBuf/ConvBuf under MutexSocketBuf/MutexConvBuf respectively (channel.cpp:226–239); SetGain/GetGain both lock Mutex.
• CThreadPool (src/threadpool.h) guards its stop flag and queue with queue_mutex + condition variable.
• The recorder runs in its own QThread and communicates via queued signals only.

Suggested direction (no patch here by design)

The bool cases are one-line conversions to std::atomic<bool> using the overloaded operators (per the conclusion in #3786, the default sequentially-consistent operations are negligible at these rates). iConTimeOut and the C-class ints would become std::atomic<int>; D3 wants either one mutex or atomics for the two size fields; C's AudioReverb.Clear() needs to move onto the audio thread or under MutexAudioProcessCallback. Happy to follow up with a PR if the direction is agreed.

[ann0see]

Thanks

[mcfnord]

Addendum to the batch-C table, found while preparing #3802: CClient::fMuteOutStreamGain (src/client.h:394) — written by the GUI thread in SetRemoteChanGain() under MutexGainOrPan (client.cpp:506), but read lock-free by the sound thread in ProcessAudioDataIntern() (client.cpp:1588). A mutex held only on the writer side synchronizes nothing, so this is the same pattern as the rest of batch C, plus the possibility of a torn float read. Included in #3802 alongside the other mechanical conversions.

[pljones]

On x86 most of these "work" today, which is why they've been harmless-looking for years.

Thanks for putting the full explanation in and why it's being seen more now.

[pljones]

I'd hesitate to raise a single PR for all the changes. One per batch, perhaps (maybe combining E with all of batch A).

One of the main issues with something like this change is measuring it:

• is the result safe?
• does the result degrade performance?
• was there a better approach?

I don't know this level of C++ implementation well enough to have answers - of verify answers - to any of the questions I want to ask!

[ann0see]

I believe that this can indeed result in degraded performance. Synchronisation always adds overhead and I would say is safer than not having it.

std::atomic is the easiest way I can think of (otherwise we'd need locks). If we don't need synchronisation guarantees in some cases, we can omit it - however we'll then need a proof that every interleaving is safe enough. Otherwise it could yield to hangs or bugs which are very hard to debug. I believe that we do have them due to Jamlus occasionally hanging (the socket bug smelled like a concurrency bug to me) if it is caused by not guarding booleans - maybe or maybe not.

[mcfnord]

I wonder what the performance hit would be if we used this everywhere we possibly could. Could be miniscule.

[pljones]

Yeah, I'm sure std::atomic is likely to be a trivial overhead. I'm more suspicious of any QAtomic or QMutux calls - they'll potentially have more wrapping layers over the lower standard C++ calls.

[ann0see]

I don't think it's trivial. It involves memory operations and likely cache flushing. https://stackoverflow.com/questions/51846894/what-is-the-performance-of-stdatomic-vs-non-atomic-variables

[mcfnord]

Update on batch F: F1 is resolved by #3788. That PR deletes CSocket::Close() entirely — with the non-blocking socket reads from #3774, the receive loops are now gated on the (atomic) bRun instead of relying on a close-while-in-use to unblock recvfrom. That removes both the unsynchronized fd overwrite described in F1 and the mechanism it rode on, so nothing in batch F remains once #3788 merges (and it has been merged).