fix(spot): defer socket construction to SpotClients thread (Windows crash)#2420
Conversation
There was a problem hiding this comment.
Thanks for the careful write-up — the QSocketNotifier construction-thread affinity gotcha on Windows is real and the symptom (cross-thread sendEvent assert during a disconnect cascade) matches the root cause exactly. The fix is consistent across all six clients and the FIFO ordering of Qt::QueuedConnection posts means initialize() is guaranteed to run before any subsequent queued connectToCluster / startListening / startPolling / startConnection call from MainWindow, so the "method called before init" race isn't a concern in practice.
A few small things worth a look:
-
Init slot called twice is silently ignored.
if (m_socket) return;in eachinitialize()makes a second call a no-op, but it doesn't log. IfMainWindowever queues init twice by accident (e.g. during a future connect/disconnect refactor), that swallow could hide the bug. AqCWarningon the early return would be cheap insurance — but optional. -
Public non-slot methods still deref unconditionally.
DxClusterClient::connectToCluster,disconnect,sendCommand,WsjtxClient::startListening,PotaClient::startPolling,FreeDvClient::startConnection, etc. all touchm_socket/m_ws/m_nam/m_pollTimerdirectly without a null check. Today they're only reachable viaQMetaObject::invokeMethod(..., QueuedConnection)fromMainWindow, so the queue-ordering invariant protects them. But the destructors andstopConnection/stopListening/stopPollinghave null guards (correctly — those can run from arbitrary threads at shutdown), so the asymmetry is a tripwire. Not a blocker, just worth a comment oninitialize()calling out the invariant — "must be queued before any other slot on this object aftermoveToThread." -
FreeDvClient's ping-timer lambda moves intoinitialize()cleanly, but them_ws->sendTextMessage("2")deref inside it is fine because the timer can't fire beforeinitialize()runs (timer is also constructed there). Worth noting in case a reviewer flags it — it's correct. -
Out of stated scope: none. All 11 files line up with the SpotClients-thread-construction story.
LGTM in substance — the build passes, the Linux/Windows smoke tests in the PR template are the right gates. Worth a quick test of a rapid radio drop/reconnect cycle on Windows to make sure the disconnect cascade really does land on SpotClients now.
…rash) On Windows, QTcpSocket / QUdpSocket / QWebSocket each create an internal QSocketNotifier whose Win32 message-loop affinity is bound to the *construction* thread, not the QObject's current thread. Because all five spot clients (DxCluster, RBN, WSJT-X, SpotCollector, POTA, FreeDV) were constructed on the main thread and then moveToThread()'d to the SpotClients worker, socket events delivered during a TCP disconnect cascade were dispatched via the main thread's event dispatcher into a QObject living on SpotClients — tripping QCoreApplication::sendEvent's cross-thread assert and killing the process. Move socket / network / timer construction into a new initialize() slot on each spot client. After moveToThread(), MainWindow invokes initialize() with Qt::QueuedConnection so it runs on the SpotClients thread before any other slot, giving the internal notifiers correct thread affinity. Linux is unaffected because epoll-based socket notifiers have no thread affinity, but the fix is portable and matches the v0.9.7 user crash report that pointed at the same code path. Fixes #1929 Fixes #2418
4275926 to
ada8119
Compare
|
Claude here — really clean root-cause analysis, jensenpat. The Win32 QSocketNotifier construction-thread affinity is the kind of cross-platform Qt quirk that's easy to miss until it bites. Deferred-init via QueuedConnection is exactly the right pattern, and the idempotent initialize() guard plus the m_listening sentinels keep the unguarded dereferences in stop paths safe. Closing #1929 and #2418 in one go is a nice bonus. Merged. 73, Jeremy KK7GWY & Claude (AI dev partner) |
…es (#2481) (#3310) Fixes #2481 ## Problem With 2 panadapters and 2 LogHX3 logs each running Spot Machine over TCI, the waterfall intermittently froze. The reporter's isolation was decisive — disabling Spot Machine in **both** logs resolved it, and turning off the verbose logs did not. So the trigger is the **spot-marker render path on the main thread**, not the TCI audio fan-out the first triage suspected. `wirePanadapter()` wired `rebuildSpots` directly to five `SpotModel` signals, **once per panadapter**. Every incoming spot emitted `spotAdded`/`spotUpdated`, firing `rebuildSpots`, which re-iterates *every* spot, runs `DxccColorProvider::colorForSpot()` (CTY resolution + QString allocations) per spot, and diffs the whole marker vector. Two simultaneous cluster feeds drove roughly **O(spots × spots/sec × pans)** main-thread work in bursts — the 10–170 ms paintEvent stalls seen in the support-bundle logs. ## Fix Coalesce the five signals through a single **50 ms** (~20 Hz, below the FFT repaint cadence) single-shot timer per pan, so a burst of N spot events collapses to **one** `rebuildSpots` per pan per window instead of N × pans. Visual output is identical, just debounced. The timer is parented to the spectrum widget (per-pan lifetime) and `QPointer`-guarded, matching the sibling `rebuildTnfMarkers` pattern. No threading or signal-routing changes. ## Files - `src/gui/MainWindow.cpp` — `wirePanadapter()` spot-signal wiring ## Structural follow-up (flagged, not done here) `colorForSpot()` / DXCC resolution still runs on the GUI thread inside `rebuildSpots`. Moving it onto the existing `SpotClients` worker thread (PR #2420 already moved the spot *sockets* there) is the larger, structural fix and a maintainer call — deliberately left out of this contained mitigation per the autonomous-agent boundaries. ## Testing Builds clean (985/985, macOS). Manual: 1. 2 panadapters + 2 LogHX3 logs (Spot Machine on) + 2 JTDX over TCI; let the cluster feeds run hot for several minutes. 2. Watch the waterfall for stutter and the title bar for "loose connection to radio" — both should be gone. 3. Confirm spots still appear/expire and DXCC colors render on both pans (refreshed at ≤20 Hz, visually indistinguishable). 💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…es (aethersdr#2481) (aethersdr#3310) Fixes aethersdr#2481 ## Problem With 2 panadapters and 2 LogHX3 logs each running Spot Machine over TCI, the waterfall intermittently froze. The reporter's isolation was decisive — disabling Spot Machine in **both** logs resolved it, and turning off the verbose logs did not. So the trigger is the **spot-marker render path on the main thread**, not the TCI audio fan-out the first triage suspected. `wirePanadapter()` wired `rebuildSpots` directly to five `SpotModel` signals, **once per panadapter**. Every incoming spot emitted `spotAdded`/`spotUpdated`, firing `rebuildSpots`, which re-iterates *every* spot, runs `DxccColorProvider::colorForSpot()` (CTY resolution + QString allocations) per spot, and diffs the whole marker vector. Two simultaneous cluster feeds drove roughly **O(spots × spots/sec × pans)** main-thread work in bursts — the 10–170 ms paintEvent stalls seen in the support-bundle logs. ## Fix Coalesce the five signals through a single **50 ms** (~20 Hz, below the FFT repaint cadence) single-shot timer per pan, so a burst of N spot events collapses to **one** `rebuildSpots` per pan per window instead of N × pans. Visual output is identical, just debounced. The timer is parented to the spectrum widget (per-pan lifetime) and `QPointer`-guarded, matching the sibling `rebuildTnfMarkers` pattern. No threading or signal-routing changes. ## Files - `src/gui/MainWindow.cpp` — `wirePanadapter()` spot-signal wiring ## Structural follow-up (flagged, not done here) `colorForSpot()` / DXCC resolution still runs on the GUI thread inside `rebuildSpots`. Moving it onto the existing `SpotClients` worker thread (PR aethersdr#2420 already moved the spot *sockets* there) is the larger, structural fix and a maintainer call — deliberately left out of this contained mitigation per the autonomous-agent boundaries. ## Testing Builds clean (985/985, macOS). Manual: 1. 2 panadapters + 2 LogHX3 logs (Spot Machine on) + 2 JTDX over TCI; let the cluster feeds run hot for several minutes. 2. Watch the waterfall for stutter and the title bar for "loose connection to radio" — both should be gone. 3. Confirm spots still appear/expire and DXCC colors render on both pans (refreshed at ≤20 Hz, visually indistinguishable). 💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
QCoreApplication::sendEventassertion that crashes AetherSDR on Windows when the radio TCP connection drops while DX Cluster / RBN / WSJT-X / SpotCollector / POTA / FreeDV clients are running on theSpotClientsworker thread.QTcpSocket/QUdpSocket/QWebSocket(and associated timers /QNetworkAccessManager) in its constructor on the main thread, thenmoveToThread()'d toSpotClients. On Windows the internalQSocketNotifierkeeps Win32 message-loop affinity to the construction thread, so socket events delivered during a disconnect cascade trip the cross-thread assert.initialize()slot on each client constructs sockets/timers;MainWindowinvokes it viaQt::QueuedConnectionaftermoveToThread()so the internal notifiers bind toSpotClients.Closes #1929
Closes #2418 (kn7k's v0.9.7 overnight crash matched the same code path — duplicate of #1929 per support-bundle analysis).
Test plan
cmake --build build -j)ASSERT failure in QCoreApplication::sendEventand no crash dialog🤖 Generated with Claude Code