jamulussoftware / jamulussoftware/jamulus
docs: write down the server's CChannel threading and locking model
Personne n'a encore pris cette issue.
- Langage dominant
- C
- Étoiles
- 1.1k
- Forks
- 248
- Merge moyen
- 2 j 3 h
- PR mergées (30 j)
- 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 bySetAddress(its only server-side caller isInitChannel); read lock-free in the mix phase (the level and recorder sends, andPrepAndSendPacket) and inGetConCliParam.CHostAddressis aQHostAddressplus a port, andQHostAddressis reference-counted, so a copy that overlapsoperator=is the same shape as theQStringcopy #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, underMutexSocketBuf) againstUpdate()from the level pass in the mix phase (no lock). Twodoubles.
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: the queued socket-to-server connection that puts protocol slots on the main thread; the OnTimer lock scope (server.cpp:669-727, mix from :729); CServer::PutAudioData and InitChannel on the socket thread; the lock-free SetAddress/GetAddress 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.
Guide de contribution
Ouvrir le guide de contribution
Par où commencer
- Lisez l'issue en entier, puis le guide de contribution du projet.
- Signalez en commentaire que vous la prenez — cela évite que deux personnes fassent le même travail.
- Forkez le dépôt et travaillez sur une branche.
- Ouvrez une pull request qui référence le numéro de l'issue.
Piste de recherche
Commencez par la docs/THREADING.md proposée et vérifiez son modèle par rapport à src/socket.cpp, src/server.cpp, src/channel.h ainsi qu’à channel.cpp et aux références Qt citées dans le commit 0545fddd. Confirmez la propriété documentée des threads et la couverture des verrous, puis ajoutez la documentation convenue sans modifier le comportement ; le travail est terminé lorsque le modèle de threading et de locking de CChannel côté serveur est consigné avec exactitude et que le périmètre de sa vérification est clair.
Rédigé par le modèle d'indexation à partir du texte de l'issue.
Évaluation
- Stack technique
- cpp
- Domaine
- backend, documentation
- Type d'issue
- Documentation
- Difficulté
- 3/5
- Temps estimé
- 1-2 jours
- Activité
- Active
- Clarté
- Clairement spécifiée
- Accessibilité débutants
- 76/100