Skip to content

Experiment: make cross-thread run/status flags std::atomic (#3798 batches A/B)#3800

Draft
mcfnord wants to merge 2 commits into
jamulussoftware:mainfrom
mcfnord:atomic-flags-3798-ab
Draft

Experiment: make cross-thread run/status flags std::atomic (#3798 batches A/B)#3800
mcfnord wants to merge 2 commits into
jamulussoftware:mainfrom
mcfnord:atomic-flags-3798-ab

Conversation

@mcfnord

@mcfnord mcfnord commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

This follows up on the discussion in #3786 and covers batches A and B of the audit in #3798 — the thread-lifecycle bRun flags and the jitter-buffer status flags. Offering it as an experiment: it is the least invasive slice of #3798, intended to establish the std::atomic pattern in the codebase and let us observe it through CI and normal use before deciding whether the more involved findings (C–F) are worth touching.

What changes

Five members become std::atomic<bool>, keeping plain operator syntax at every call site (per the conclusion in #3786 that the overloaded operators are simplest, and the default sequentially-consistent operations cost nothing at these rates):

Member Written by Read by
CHighPrecisionTimer::bRun (Mac/Linux variant) main thread (Start()/Stop()) timer thread loop; socket thread via IsRunning()
CSoundBase::bRun main thread (Start()/Stop()) audio callback thread (JACK/CoreAudio/Oboe/ASIO)
CSoundBase::bCallbackEntered audio callback thread GUI sound-device check
CSocket::bJitterBufferOK socket thread GUI read-and-reset
CClient::bJitterBufferOK sound thread GUI read-and-reset

A sixth member, CSocketThread::bRun (the exact #3786 case), was originally part of this PR but has been dropped: #3788 already converts it — and removes CSocket::Close() in favour of non-blocking reads, which also disposes of finding F1 in #3798. The <atomic> include added to socket.h here matches #3788's placement so the two branches merge cleanly in either order.

The only non-declaration change: the two GetAndResetbJitterBufferOKFlag() functions now use exchange ( true ) instead of a separate read and reset. Behavior is the same, and it closes a small lost-update window — a "not OK" written by the socket/sound thread between the read and the reset used to be dropped silently.

Behavior

No intended behavior change. On x86 these compile to essentially the same instructions; the difference is that the compiler may no longer cache the flag across loop iterations, and on weakly-ordered CPUs (Apple Silicon, Android, ARM Linux servers) the cross-thread store is guaranteed to become visible and correctly ordered.

Tested

  • Full Linux client+server build (Qt 5.15, JACK enabled): compiles with no new warnings.
  • Headless server smoke test: starts, runs, exits cleanly on SIGTERM — this exercises the modified Stop() paths of the socket and timer threads.
  • macOS / Windows / Android builds not tested locally (relying on CI); the touched accesses in those backends are plain reads/writes covered by the operator overloads.

🤖 Generated with Claude Code

Comment thread src/socket.h Outdated
CSocket* pSocket;
bool bRun;
CSocket* pSocket;
std::atomic<bool> bRun;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this one is already addressed as part of #3788, which is just awaiting a second approval.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — dropped from this PR in favour of #3788 (kept the bJitterBufferOK conversion in the same file, and matched your <atomic> include placement so the branches merge cleanly in either order). Nice bonus: removing CSocket::Close() also disposes of finding F1 in #3798.

@ann0see

ann0see commented Jul 18, 2026

Copy link
Copy Markdown
Member

Also you need to fix the coding style violations

The socket, high-precision timer and sound threads are stopped by a
plain bool written from another thread, and the jitter-buffer status
flags are written and read/reset from different threads. Convert these
six flags to std::atomic<bool>:

- CSocketThread::bRun
- CHighPrecisionTimer::bRun (Mac/Linux variant)
- CSoundBase::bRun
- CSoundBase::bCallbackEntered
- CSocket::bJitterBufferOK
- CClient::bJitterBufferOK

The read-and-reset functions now use exchange() so a "not OK" written
between a separate read and reset can no longer be lost.

Addresses batches A and B of jamulussoftware#3798.

Co-Authored-By: Claude Fable 5 <[email protected]>
@mcfnord
mcfnord force-pushed the atomic-flags-3798-ab branch from 3903e84 to e12cd8d Compare July 18, 2026 14:14
@mcfnord

mcfnord commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Also you need to fix the coding style violations

This error class should be prevented when help about the formatting rules (or a link to the rules) appears in AGENTS.md. I don't see your draft of AGENTS.md as a PR, and I think you said you'd be providing one, so I remember yesterday my agent linked to my draft in a comment. For now, my CLAUDE.md has added the following guidance, including the note that eventually this will reach the AGENTS.md file when it exists:

Upstream PRs (jamulussoftware/jamulus)

Upstream CONTRIBUTING.md is not auto-loaded context — read it before starting any upstream-bound change. (This section migrates to upstream AGENTS.md when that lands; the draft on the agent-deploy-docs branch already carries these rules, and PR #3793 adds the CONTRIBUTING.md pointer to it.)

Style, before every commit: clang-format-14 -i on all touched .cpp/.h/.mm (installed locally 2026-07-18; CI pins clang-format 14 in .github/workflows/coding-style-check.yml). make clang_format is the repo target but invokes unversioned clang-format, which this box doesn't have. Beware AlignConsecutiveDeclarations: widening one declaration's type re-pads its neighbors — this cost PR #3800 a red X.

softins already makes this member atomic in jamulussoftware#3788 (which also removes
CSocket::Close in favour of non-blocking reads). Keep only the
bJitterBufferOK conversion in this file; the <atomic> include moves to
match jamulussoftware#3788 so the two branches merge cleanly in either order.

Co-Authored-By: Claude Fable 5 <[email protected]>
Comment thread src/socket.cpp
// atomically read the jitter buffer status and reset it to OK so that a
// "not OK" set by the socket thread between a separate read and reset
// cannot be lost
return bJitterBufferOK.exchange ( true );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice, and more concise than the original!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants