Make cross-thread audio params and channel/server flags std::atomic#3802
Open
mcfnord wants to merge 1 commit into
Open
Make cross-thread audio params and channel/server flags std::atomic#3802mcfnord wants to merge 1 commit into
mcfnord wants to merge 1 commit into
Conversation
Second slice of jamulussoftware#3798 (batches C, D, E — mechanical conversions only): GUI-thread parameters read by the audio callback, client channel state read by the socket thread, and the server disconnect flag written by thread-pool workers. The comment at the disconnect flag claimed the write was atomic; it now is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
This is likely the comment I read a while ago which made me suspicious. |
Collaborator
I mean, this is fundamentally broken... What does it do if there isn't a GUI? State shouldn't in the GUI. I'd rather the right fix was put in: use signals to pass state out of the GUI (not hold anything but the visual representation), respond to signals of change in state from the Client. Or does this just mean "on the main thread"? |
mcfnord
marked this pull request as ready for review
July 18, 2026 18:27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second slice of the audit in #3798, following the same experimental approach as #3800: only the mechanical conversions from batches C (GUI-thread parameters read by the audio callback), D (client-side channel state), and E (the server disconnect flag). Everything that needs an actual design decision is deliberately left out — see "Not included" below.
What changes
Nine members become
std::atomic, keeping plain operator syntax at every call site (same rationale as #3800: per #3786, the overloaded operators are the simplest form, and default sequentially-consistent operations are negligible at these rates). No control flow changes at all this time — every access stays a plain load or store through the operators.CClient::bMuteOutStreamCClient::fMuteOutStreamGainSetRemoteChanGain, underMutexGainOrPan)CClient::iAudioInFaderCClient::bReverbOnLeftChanCClient::iReverbLevelCClient::iInputBoostCChannel::bIsEnabledMutex)PutAudioData(), no lockCChannel::iConTimeOutDisconnect(), no lock)MutexSocketBuf),IsConnected()readersCServer::bChannelIsNowDisconnectedfuture.wait()fMuteOutStreamGainis an addendum to the batch-C table in #3798, found while preparing this patch: it is written by the GUI thread underMutexGainOrPanbut read lock-free by the sound thread inProcessAudioDataIntern()— a writer-side-only mutex synchronizes nothing, and theintmembers in the same table can additionally tear. It is the same mechanical pattern, so it is included here.The one comment change: the note above the
bChannelIsNowDisconnectedstore (server.cpp) claimed "no mutex is needed … an atomic write". As discussed in #3786, a plainboolstore is not an atomic write in the C++ memory model. With the member nowstd::atomic<bool>, the comment is reworded to be true.Not included (needs discussion, not mechanics)
SetReverbOnLeftChan()also callsAudioReverb.Clear()on the GUI thread while the sound thread may be insideAudioReverb.Process()— concurrent mutation of filter state. The clean fix (deferring the clear onto the audio thread) changes behavior slightly and deserves its own review.iNetwFrameSize/iNetwFrameSizeFactare written underMutexbut read underMutexSocketBuf; fixing that is a locking-design choice.Close()fd overwrite race is the intended unblock mechanism and needs a considered design if it is to change at all.Behavior
No intended behavior change. The
iConTimeOut -= ninPutAudioData()becomes an atomicfetch_sub, but that path already runs underMutexSocketBuf, so ordering there is unchanged; the fix is for the lock-free writers/readers elsewhere.Tested
clang-format-14clean.OnHandledSignal: 15).Note for whoever merges: #3800 also adds
#include <atomic>tosrc/client.h, so whichever of the two lands second will need a trivial rebase of that one line.🤖 Generated with Claude Code