jamulussoftware / jamulussoftware/jamulus

docs: write down the server's CChannel threading and locking model

Open
#3,933 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
1.1k
Forks
248
Avg merge
2d 3h
Merged PRs (30d)
9

Description

**🤖 AI:** A server `CChannel` is shared between the main thread and the socket thread, and nothing in the tree says which lock covers which member - #3930 and #3932 were both instances of that gap. Below is that model written down as the code implements it, proposed as `docs/THREADING.md` beside `docs/JAMULUS_PROTOCOL.md` (or as a section of an existing file, if a new one is unwelcome). It adds no behaviour. The one finding in it that is more than bookkeeping: with the thread map measured, only two members are left unprotected, and one of them - `InetAddr` - fails the same way the #3930 channel-name bug did.

Proposed content:

---

# Server threading and channel locking

Two threads touch a server `CChannel` concurrently, and this note writes down which lock covers which member. It describes the code as it is; it adds no behaviour. (Scope: the Linux server without `--multithreading`; see Verification at the end.)

The **main thread** does almost everything. `CServer::OnTimer()` runs here (the timer object emits from its own `QThread`, and the default `Qt::AutoConnection` queues the slot to the thread `CServer` lives on). Protocol handling runs here too: `CSocket` emits `ProtocolMessageReceived` from the socket thread, the queued connection delivers it to `CServer::OnProtocolMessageReceived` on the main thread, and that call - holding `CServer::Mutex` - drives every protocol slot of `CChannel`: `SetChanInfo`, `SetGain`/`SetPan`, `OnNetTranspPropsReceived`, `OnVersionAndOSReceived`, `OnJittBufSizeChange`. JSON-RPC handlers also run on the main thread and read channels through `CServer::GetConCliParam()`.

The **socket thread** (`CSocketThread`) does exactly one thing: `CServer::PutAudioData()`, which takes `CServer::Mutex`, feeds each incoming audio packet to `CChannel::PutAudioData()`, and - when a packet arrives from a new address - initialises a channel in `CServer::InitChannel()`: `SetAddress`, `ResetInfo`, `SetGain`/`SetPan`.

The **recorder** (`JamRecorder`) receives `AudioFrame` over a queued connection with copied arguments and shares no channel state.

## One timer tick

`CServer::Mutex` is the boundary between the two threads. `OnTimer()` holds it for the first half of the tick and releases it before the second:

```
main thread |== CServer::Mutex held ==========|== released =================|
(one tick) | collect connected channels | channel levels |
| decode (DecodeReceiveData) | mix + send |
| | (MixEncodeTransmitData, |
| | PrepAndSendPacket) |
socket thread | a packet arriving here blocks | PutAudioData / InitChannel |
| on CServer::Mutex | run IN PARALLEL with mix |

```

So the racy question is always the same one: **what does the socket thread write, and does the mix phase or an RPC handler read it without a common lock?** Everything else is serialised - either both sides hold `CServer::Mutex`, or both sides are the main thread.

## What a reader must hold, member by member

| Member(s) | Writers (thread) | A reader on another thread must |
|---|---|---|
| `ChannelInfo`, incl. the channel name | `ResetInfo` (socket), `SetChanInfo` (main) | take `Mutex` - `GetName`/`GetChanInfo` do (since #3930) |
| `bIsIdentified` | same writers | nothing - `std::atomic` (since #3932) |
| `vecfGains`, `vecfPannings` | `InitChannel` (socket), protocol slots (main) | take `Mutex` - `GetGain`/`GetPan` do |
| `SockBuf` contents, `iFadeInCnt` | `PutAudioData` (socket) | take `MutexSocketBuf`, or read in the decode phase under `CServer::Mutex` (what `GetFadeInGain` relies on) |
| `iConTimeOut` | `PutAudioData` (socket) | nothing - `std::atomic` (`IsConnected`) |
| `InetAddr` | `SetAddress` (socket) | **nothing exists - gap, see below** |
| `SignalLevelMeter` | `Reset` in `PutAudioData` (socket) | **nothing exists - gap, see below** |
| transport properties (`eAudioCompressionType`, `iNumAudioChannels`, `iNetwFrameSize`, `iNetwFrameSizeFact`, `iCeltNumCodedBytes`, `iAudioFrameSizeSamples`, `iFadeInCntMax`), `iCurSockBufNumFrames`, `bDoAutoSockBufSize`, `bUseSequenceNumber`, `ConvBuf`, `iSendSequenceNumber` | main thread only | nothing extra today - the socket thread's few reads of them in `PutAudioData` are under `CServer::Mutex`, which the writers hold; the lock-free inline getters are safe because no second thread calls them |
| `bIsServer`, `iConTimeOutStartVal` | constructor only | nothing |

## The two gaps

- **`InetAddr`** - written lock-free by `SetAddress` (its only server-side caller is `InitChannel`); read lock-free in the mix phase (the level and recorder sends, and `PrepAndSendPacket`) and in `GetConCliParam`. `CHostAddress` is a `QHostAddress` plus a port, and `QHostAddress` is reference-counted, so a copy that overlaps `operator=` is the same shape as the `QString` copy #3930 fixed: of the two gaps, this is the one whose outcome is a use-after-free rather than a stale value. Written once per new connection.
- **`SignalLevelMeter`** - `Reset()` on a new connection (socket thread, under `MutexSocketBuf`) against `Update()` from the level pass in the mix phase (no lock). Two `double`s.

## Verification

Verified at commit `0545fddd` (carries #3930 and #3932), on Linux, Qt 5.15.13. Thread attribution is measured, not inferred from the connect calls: in ThreadSanitizer runs under 8-client connection churn, `OnTimer` and `OnProtocolMessageReceived` appear only on the main thread, `PutAudioData` and `InitChannel` only on `CSocketThread`; gdb breakpoints on a live server show `PutAudioData` on thread 2 (`CSocketThread`) and `OnTimer`, `SetChanInfo` and the JSON-RPC path into `GetConCliParam` on thread 1. A QMutex-aware ThreadSanitizer build reports the `InetAddr` pair (once in 300 s of churn); the same churn under AddressSanitizer with recording enabled produced no report in 1508 connections over 600 s. Not measured, so not covered here: the client's use of `CChannel`, Windows, macOS, the GUI dialog, and `--multithreading`.

---

For review, the load-bearing anchors at [`0545fddd`](https://github.com/jamulussoftware/jamulus/commit/0545fddd715644f9615258f135515cc6c1406379): the [queued socket-to-server connection](https://github.com/jamulussoftware/jamulus/blob/0545fddd715644f9615258f135515cc6c1406379/src/socket.cpp#L134) that puts protocol slots on the main thread; the [`OnTimer` lock scope](https://github.com/jamulussoftware/jamulus/blob/0545fddd715644f9615258f135515cc6c1406379/src/server.cpp#L652-L727) (`server.cpp:669`-`727`, mix from `:729`); [`CServer::PutAudioData`](https://github.com/jamulussoftware/jamulus/blob/0545fddd715644f9615258f135515cc6c1406379/src/server.cpp#L1584-L1607) and [`InitChannel`](https://github.com/jamulussoftware/jamulus/blob/0545fddd715644f9615258f135515cc6c1406379/src/server.cpp#L1490-L1510) on the socket thread; the lock-free [`SetAddress`/`GetAddress`](https://github.com/jamulussoftware/jamulus/blob/0545fddd715644f9615258f135515cc6c1406379/src/channel.h#L109-L110) pair (`channel.h:109`-`110`, read at `server.cpp:748`/`:756`/`:1628`; `SignalLevelMeter` at `channel.cpp:636`/`:738`; `qhostaddress.h:160` is the `QExplicitlySharedDataPointer`).

If the mapping is right, a PR adding the file can follow. Whether `InetAddr` gets a fix, and of which shape, is the question the mapping raises.

---

🤖 *This message was written by AI and reviewed by @mcfnord.*

Contributor guide

Open the contributing guide

Research direction

Start with the proposed docs/THREADING.md and verify its model against src/socket.cpp, src/server.cpp, src/channel.h, and the cited channel.cpp and Qt references at commit 0545fddd. Confirm the documented thread ownership and lock coverage, then add the agreed documentation without changing behavior; done means the server CChannel threading and locking model is recorded accurately and its verification scope is clear.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, documentation
Issue type
Documentation
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.