jamulussoftware / jamulussoftware/jamulus

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

オープン
#3,933 コメント 2 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

主要言語
C
スター
1.1k
フォーク
248
平均マージ
2日 3時間
マージ済み PR(30日)
9

説明

**🤖 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.*

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

提案されている docs/THREADING.md から始め、そのモデルを src/socket.cpp、src/server.cpp、src/channel.h、および commit 0545fddd にある引用された channel.cpp と Qt の参照に照らして検証します。文書化されたスレッドの所有権とロックの適用範囲を確認し、動作を変更せずに合意されたドキュメントを追加します。サーバーの CChannel のスレッド処理およびロックのモデルが正確に記録され、その検証範囲が明確になれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
cpp
領域
backend, documentation
issue の種類
ドキュメント
難易度
3/5
見積もり時間
1〜2日
活発さ
活発
明瞭さ
明確に書かれている
初心者へのやさしさ
76/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。