Harden ASIO re-init: ignore stray bufferSwitch() when not running; narrow ASIOMutex scope

[ann0see]

Summary

Possible hardening: make CSound::bufferSwitch() ignore stray callbacks when
not running, and narrow the ASIOMutex scope in CSound::Init(). This is a
proposed improvement only — it is not the fix for the #3779 hang (that is
PR #3867, a Qt::QueuedConnection change). It is a separate, optional
measure for a distinct ASIOMutex race that #3867 does not touch.

Background

• Investigate Windows Hang if using ASIO4ALL especially #3779 (Windows hang with ASIO4ALL) turned out to be a MutexDriverReinit
  deadlock, fixed by Avoid windows ASIO4ALL hang by using QueuedConnection #3867. There is also an ASIOMutex-related race which
  this issue covers.
• A similar narrow-lock approach was tried in Try to fix ASIO4ALL hang #3862 and closed as insufficient
  on its own; the current proposal is a secondary measure, not a replacement.

Proposed change (idea — may not be correct)

1. CSound::bufferSwitch() — early-return when not running

src/sound/asio/sound.cpp:

 void CSound::bufferSwitch ( long index, ASIOBool )
 {
+    // if not running, ignore stray callbacks (e.g. from ASIO4ALL's thread
+    // during Init()/teardown) so they cannot block on ASIOMutex
+    if ( !pSound->bRun )
+    {
+        return;
+    }
+
     int iCurSample;

2. CSound::Init() — narrow the ASIOMutex scope

Keep the lock only around the vecsMultChanAudioSndCrd.Init() reallocation and
move the opaque driver calls (ASIOSetSampleRate, ASIODisposeBuffers,
ASIOCreateBuffers, ASIOGetLatencies, ASIOOutputReady) outside it:

 int CSound::Init ( const int iNewPrefMonoBufferSize )
 {
-    ASIOMutex.lock(); // get mutex lock
-    {
-        // get the actual sound card buffer size which is supported
-        // by the audio hardware
-        iASIOBufferSizeMono = GetActualBufferSize ( iNewPrefMonoBufferSize );
+    // get the actual sound card buffer size which is supported
+    // by the audio hardware
+    iASIOBufferSizeMono = GetActualBufferSize ( iNewPrefMonoBufferSize );
 
-        // init base class
-        CSoundBase::Init ( iASIOBufferSizeMono );
+    // init base class
+    CSoundBase::Init ( iASIOBufferSizeMono );
 
-        // set internal buffer size value and calculate stereo buffer size
-        iASIOBufferSizeStereo = 2 * iASIOBufferSizeMono;
+    // set internal buffer size value and calculate stereo buffer size
+    iASIOBufferSizeStereo = 2 * iASIOBufferSizeMono;
 
-        // set the sample rate
-        ASIOSetSampleRate ( SYSTEM_SAMPLE_RATE_HZ );
+    // set the sample rate
+    ASIOSetSampleRate ( SYSTEM_SAMPLE_RATE_HZ );
 
-        // create memory for intermediate audio buffer
+    // create memory for intermediate audio buffer. This reallocation is the
+    // only part which must not race with a running audio callback, so only it
+    // is guarded by the mutex. The opaque driver calls are deliberately kept
+    // outside the mutex: a driver may block inside them until its audio thread
+    // returns from bufferSwitch(), which would deadlock if the mutex were held
+    // here (see issue #3779)
+    ASIOMutex.lock(); // get mutex lock
+    {
         vecsMultChanAudioSndCrd.Init ( iASIOBufferSizeStereo );
+    }
+    ASIOMutex.unlock();
 
-        // create and activate ASIO buffers (buffer size in samples),
-        // dispose old buffers (if any)
-        ASIODisposeBuffers();
-
-        // prepare input channels
-        for ( int i = 0; i < lNumInChan; i++ )
-        {
-            bufferInfos[i].isInput    = ASIOTrue;
-            bufferInfos[i].channelNum = i;
-            bufferInfos[i].buffers[0] = 0;
-            bufferInfos[i].buffers[1] = 0;
-        }
+    // create and activate ASIO buffers (buffer size in samples),
+    // dispose old buffers (if any)
+    ASIODisposeBuffers();
 
-        // prepare output channels
-        for ( int i = 0; i < lNumOutChan; i++ )
-        {
-            bufferInfos[lNumInChan + i].isInput    = ASIOFalse;
-            bufferInfos[lNumInChan + i].channelNum = i;
-            bufferInfos[lNumInChan + i].buffers[0] = 0;
-            bufferInfos[lNumInChan + i].buffers[1] = 0;
-        }
+    // prepare input channels
+    for ( int i = 0; i < lNumInChan; i++ )
+    {
+        bufferInfos[i].isInput    = ASIOTrue;
+        bufferInfos[i].channelNum = i;
+        bufferInfos[i].buffers[0] = 0;
+        bufferInfos[i].buffers[1] = 0;
+    }
 
-        ASIOCreateBuffers ( bufferInfos, lNumInChan + lNumOutChan, iASIOBufferSizeMono, &asioCallbacks );
+    // prepare output channels
+    for ( int i = 0; i < lNumOutChan; i++ )
+    {
+        bufferInfos[lNumInChan + i].isInput    = ASIOFalse;
+        bufferInfos[lNumInChan + i].channelNum = i;
+        bufferInfos[lNumInChan + i].buffers[0] = 0;
+        bufferInfos[lNumInChan + i].buffers[1] = 0;
+    }
 
-        // query the latency of the driver
-        long lInputLatency  = 0;
-        long lOutputLatency = 0;
+    ASIOCreateBuffers ( bufferInfos, lNumInChan + lNumOutChan, iASIOBufferSizeMono, &asioCallbacks );
 
-        if ( ASIOGetLatencies ( &lInputLatency, &lOutputLatency ) != ASE_NotPresent )
-        {
-            // add the input and output latencies (returned in number of
-            // samples) and calculate the time in ms
-            fInOutLatencyMs = ( static_cast<float> ( lInputLatency ) + lOutputLatency ) * 1000 / SYSTEM_SAMPLE_RATE_HZ;
-        }
-        else
-        {
-            // no latency available
-            fInOutLatencyMs = 0.0f;
-        }
+    // query the latency of the driver
+    long lInputLatency  = 0;
+    long lOutputLatency = 0;
 
-        // check whether the driver requires the ASIOOutputReady() optimization
-        // (can be used by the driver to reduce output latency by one block)
-        bASIOPostOutput = ( ASIOOutputReady() == ASE_OK );
+    if ( ASIOGetLatencies ( &lInputLatency, &lOutputLatency ) != ASE_NotPresent )
+    {
+        // add the input and output latencies (returned in number of
+        // samples) and calculate the time in ms
+        fInOutLatencyMs = ( static_cast<float> ( lInputLatency ) + lOutputLatency ) * 1000 / SYSTEM_SAMPLE_RATE_HZ;
+    }
+    else
+    {
+        // no latency available
+        fInOutLatencyMs = 0.0f;
     }
-    ASIOMutex.unlock();
+
+    // check whether the driver requires the ASIOOutputReady() optimization
+    // (can be used by the driver to reduce output latency by one block)
+    bASIOPostOutput = ( ASIOOutputReady() == ASE_OK );
 
     return iASIOBufferSizeMono;
 }

Why the narrowing should be safe (and what to double-check)

Everything bufferSwitch() reads once the lock is dropped — iASIOBufferSizeMono,
iASIOBufferSizeStereo, vecsMultChanAudioSndCrd — is written before the
mutex is taken and is not touched again during the unlocked driver calls. So
shrinking the lock should not expose torn state: a stray callback sees either
the old vector (fully written, from the previous Init) or the new one (fully
written, under the lock), never a half-reallocated one. That said, this is an
argument from reading the code, not from instrumentation — I'd want a reviewer
to confirm there's no other writer to those members on the re-init path.

Caveats (why this may be wrong)

• bRun semantics and the thread-safety of reading it from the driver callback
  thread need review (it is std::atomic<bool> on main).
• Whether a stray callback can actually fire during the Init()/teardown
  window is not proven on all drivers; ASIO4ALL keeps its thread alive while
  not streaming, which is why it is suspected here.
• The narrowing could in principle introduce a data race on
  vecsMultChanAudioSndCrd in a path not considered here.

Requested actions

• Review whether the !pSound->bRun early-return and/or the ASIOMutex
  narrowing are correct and worth adding.
• If agreed, implement as a small change on main and test on Windows with
  ASIO4ALL.

⚠️ AI-generated issue — please verify

• This text is AI generated, may be wrong, and may contain inaccuracies.

[pljones]

How does this get communicated to the UI buffer size display?

(Where's the JSONRPC sound support, eh? Hard work to get that right cross platform... But it would make testing so much easier. Running a headless Client and using JSONRPC to manipulate the sound settings to see when things break independently of the UI thread model Qt imposes would possibly reveal a lot.)

[ann0see]

Running a headless Client and using JSONRPC to manipulate the sound settings to see when things break independently of the UI thread model Qt imposes would possibly reveal a lot.

Yes. Very likely. I had a local build where JSON-RPC could open the ASIO window automatically for example. But it's highly likely hacky if we instrument all settings

[ann0see]

How does this get communicated to the UI buffer size display?

Do you think that we can verify this by looking at the buffer size buttons, in the UI, the transfer rate, the network buffers (probably not?) or what else to look for?

[pljones]

I'm trying to think if there was already a signal/slot somewhere that handled this. It would be on the Audio/Network Settings for the display side.

[mcfnord]

Clues suggest testing on Windows would help here?

MY LLM WROTE:

I'm trying to think if there was already a signal/slot somewhere that handled this.

Not for the buffer-delay frame. CClientSettingsDlg has exactly one connection from the client, MidiCCReceived (clientsettingsdlg.cpp:950). UpdateSoundCardFrame() (:1087) is reachable only from UpdateDisplay(), whose non-user callers are showEvent and TimerStatus at DISPLAY_UPDATE_TIME = 1000 ms — so it is a 1 Hz poll.

The nearest existing hook is CClient::SoundDeviceChanged (clientdlg.cpp:552 -> :1203), which already reaches into the dialog, but only for UpdateSoundDeviceChannelSelectionFrame() — the device and channel combos, not the buffer frame.

Do you think that we can verify this by looking at the buffer size buttons, in the UI, the transfer rate, the network buffers (probably not?) or what else to look for?

Two of the three work. Measured on the shipped GUI against the JACK dummy driver, whose bufferSizeCallback enters the same EmitReinitRequestSignal ( RS_ONLY_RESTART_AND_INIT ) that kAsioBufferSizeChange does, with everything from OnSndCrdReinitRequest on being platform-independent. Client connected, period changed from outside it, dialog open and untouched:

driver period	Buffer Delay	Audio Stream Rate	Jitter Buffer local/server
128	5.33 ms (128) checked	480 kbps	2 / 2
256	10.67 ms (256) checked	364 kbps	3 / 4
back to 128	5.33 ms (128) checked	480 kbps	5 / 3

The buttons and the stream rate both identify the size and both return to the same value. The jitter buffer moves too, but it is the auto sizer reacting and it does not repeat between identical states, so it identifies nothing — the "probably not" was right. The stream rate reads --- while disconnected.

A fourth indicator covers the case the buttons cannot show: at an unsupported size the frame title itself carries the number. At period 96, disconnected, it reads Buffer Delay: 4.00 ms (96) with all three buttons greyed and none checked, and it goes back to a plain Buffer Delay on a change to 128.

On the patch itself: the early-return is a prerequisite for the narrowing, not an alternative to it. The narrowing moves ASIODisposeBuffers()/ASIOCreateBuffers() and the iASIOBufferSizeMono write outside the lock that currently keeps a concurrent bufferSwitch() off disposed buffers, and off a vecsMultChanAudioSndCrd whose length no longer matches that size. The early-return is what removes the exposure. Consistency also favours it: ASIO is the only backend whose callback has no not-running guard (JACK jack/sound.cpp:368, CoreAudio coreaudio-mac/sound.cpp:1026, Oboe oboe/sound.cpp:225) and the only one that never takes MutexAudioProcessCallback, so CSoundBase::Stop()'s drain does nothing there.

[pljones]

The issue here is the size might be 2048 or some other undefined value -- we need to ensure that's handled correctly in the GUI. I'm pretty sure it is, I just don't remember how.