Repository: orottier/web-audio-api-rs (language: python)

Issue:
Suspended + Sound put windows apps into "Not Responding" state
This one I can actually easily recreate with a test!  Yes, this code is slop but it's just a test file I hack away at when I find something I need to test outside the confines of my very complicated game code.  The actual preproduction would probably be a lot simplier.

This one is easy -- is you suspend_sync() and then play a sound (it might take one or two) eventually a windows application will fall into "not responding" and lock up.  No processor time, BTW, just it locks up.  Could be some interaction with winit.  I found this on the rc.5 branch.

`use winit::{event::*, event_loop::{ControlFlow, EventLoop}, window::{Window, WindowBuilder}};
use web_audio_api::{context::{BaseAudioContext, AudioContext}, node::{AudioNode, AudioScheduledSourceNode, PanningModelType, DistanceModelType}};
use std::f32::consts::PI;
use std::time::Instant;

pub async fn run() {
    env_logger::init();

    let event_loop = EventLoop::new();
    let window =
        WindowBuilder::new()
            .with_title("blech")
            .with_inner_size(winit::dpi::LogicalSize::new(1024.0, 768.0))
            .with_resizable(false)
            .build(&event_loop).unwrap();

    // the audio context
    let ctx = AudioContext::default();

    // run the window loop
    event_loop.run(move |event, _, control_flow| {
        match event {
            Event::WindowEvent {
                ref event,
                window_id,
            } if window_id == window.id() => {
                match event {
                    WindowEvent::MouseInput {
                        button,
                        state,
                        ..
                    } => {
                        if *state==ElementState::Pressed {
                            println!("sound");

                            ctx.suspend_sync();

                            let length = ctx.sample_rate() as usize;
                            let sample_rate = ctx.sample_rate();
                            let mut src = ctx.create_buffer_source();
                            let flat_wave = vec![1.0; length];
                            let mut buffer = ctx.create_buffer(1, length, sample_rate);
                            buffer.copy_to_channel(&flat_wave, 0);
                            
                            src.set_buffer(buffer.clone());
                            src.set_loop(true);

                            let gain = ctx.create_gain();
                            gain.gain().set_value(1.0);

                            src.connect(&gain);
                            gain.connect(&ctx.destination());
                            src.start();

                        }
                    },
                    WindowEvent::CloseRequested => *control_flow = ControlFlow::Exit,
                    _ => {}
                }
            }
            Event::RedrawRequested(window_id) if window_id == window.id() => {
            }
            Event::RedrawEventsCleared => {
                window.request_redraw();
            }
            _ => {}
        }
    });
}`


Discussion / hints:
Hi @ggadwa. Thanks for another report, I really appreciate it.

I don't have a Windows machine laying around here so I can't run your code, but I can think of a possible cause.

Our library may block the control thread when the render thread cannot handle control messages fast enough. Control messages are actions like "add a new node", "start a node", "change a param value" etc.
The queue size of control messages is 256 items so in normal operations we can handle that many messages per render block (typically run every 3 milliseconds or so).
When the context is suspended, this may become an issue because we will block indefinitely after 256 new messages as the render thread handles zero messages in suspended state.
We don't use an unbounded (infinite size) channel because it can cause (de)allocations in the render thread which is a no go for latency considerations.

Now looking at your example code, it does not really seem like you are emitting this many control messages so maybe there is something else going on. Anyway I would like to rule out this scenario. So if you are okay with it, I will push a new branch (you know the drill) and I'm happy to hear the output.
I have pushed a branch called `feature/crash-on-full-channel`, it would be very helpful if you could run your code with it
Here you go!

[2024-04-17T19:33:04Z INFO  web_audio_api::io::cpal] Audio Output Host: cpal Wasapi
[2024-04-17T19:33:04Z INFO  web_audio_api::io::cpal] Output device: Ok("Speakers / Headphones (Realtek Audio)")
[2024-04-17T19:33:04Z DEBUG web_audio_api::io::cpal] Attempt output stream with preferred config: StreamConfig { channels: 2, sample_rate: SampleRate(48000), buffer_size: Fixed(128) }
[2024-04-17T19:33:04Z INFO  web_audio_api::render::thread] Entering garbage collector thread
[2024-04-17T19:33:04Z DEBUG web_audio_api::io::cpal] Output stream set up successfully
[2024-04-17T19:33:04Z DEBUG web_audio_api::events] Entering event thread
click
before suspend_sync
[2024-04-17T19:33:06Z DEBUG web_audio_api::context::online] suspend_sync called
[2024-04-17T19:33:06Z DEBUG web_audio_api::context::online] Supending audio graph, waiting for signal..
[2024-04-17T19:33:06Z DEBUG web_audio_api::context::online] Supended audio graph. Suspending audio stream..
[2024-04-17T19:33:06Z DEBUG web_audio_api::context::online] Supended audio stream
after suspend_sync
before make sound
after make sound
click
before suspend_sync
[2024-04-17T19:33:08Z DEBUG web_audio_api::context::online] suspend_sync called
[2024-04-17T19:33:08Z DEBUG web_audio_api::context::online] Supending audio graph, waiting for signal..
error: process didn't exit successfully: `target\release\webaudio_test.exe` (exit code: 0xcfffffff)

NOTES: I added the log items so you could see where it was.  It obviously goes into the suspend_sync on the 2nd click and never comes out again (the error is when I kill the app because it's frozen.)

So: (1) suspend (2) play sound (3) suspend (freeze)

So it is suspending an already suspended context, but that should probably be a no-op.
Ahh I see, we do not handle double suspend properly and end up blocking. This should be an easy fix (hopefully this week)
I know you know this, but while you are at it, make sure double resumes are no-ops, too, just in case!

Again, thanks for this great library.  When my game enters beta you can have a beta key!
> double resumes are no-ops

Thanks for the nudge. And I would be very interested in seeing your final product

Fix patch:
diff --git a/src/context/concrete_base.rs b/src/context/concrete_base.rs
index 8f654aea..1094046e 100644
--- a/src/context/concrete_base.rs
+++ b/src/context/concrete_base.rs
@@ -351,10 +351,8 @@ impl ConcreteBaseAudioContext {
 
     /// Updates state of current context
     pub(super) fn set_state(&self, state: AudioContextState) {
-        // Only to be used from OfflineAudioContext, the online one should emit the state changes
-        // from the render thread
-        debug_assert!(self.offline());
-
+        // Only used from OfflineAudioContext or suspended AudioContext, otherwise the state
+        // changed are spawned from the render thread
         let current_state = self.state();
         if current_state != state {
             self.inner.state.store(state as u8, Ordering::Release);
diff --git a/src/context/online.rs b/src/context/online.rs
index 43ce643f..17558788 100644
--- a/src/context/online.rs
+++ b/src/context/online.rs
@@ -279,7 +279,9 @@ impl AudioContext {
     /// is currently not implemented.
     #[allow(clippy::needless_collect, clippy::missing_panics_doc)]
     pub fn set_sink_id_sync(&self, sink_id: String) -> Result<(), Box<dyn Error>> {
+        log::debug!("SinkChange requested");
         if self.sink_id() == sink_id {
+            log::debug!("SinkChange: no-op");
             return Ok(()); // sink is already active
         }
 
@@ -287,13 +289,16 @@ impl AudioContext {
             Err(format!("NotFoundError: invalid sinkId {sink_id}"))?;
         };
 
+        log::debug!("SinkChange: locking backend manager");
         let mut backend_manager_guard = self.backend_manager.lock().unwrap();
         let original_state = self.state();
         if original_state == AudioContextState::Closed {
+            log::debug!("SinkChange: context is closed");
             return Ok(());
         }
 
         // Acquire exclusive lock on ctrl msg sender
+        log::debug!("SinkChange: locking message channel");
         let ctrl_msg_send = self.base.lock_control_msg_sender();
 
         // Flush out the ctrl msg receiver, cache
@@ -303,6 +308,8 @@ impl AudioContext {
         let graph = if matches!(pending_msgs.first(), Some(ControlMessage::Startup { .. })) {
             // Handle the edge case where the previous backend was suspended for its entire lifetime.
             // In this case, the `Startup` control message was never processed.
+            log::debug!("SinkChange: recover unstarted graph");
+
             let msg = pending_msgs.remove(0);
             match msg {
                 ControlMessage::Startup { graph } => graph,
@@ -310,6 +317,8 @@ impl AudioContext {
             }
         } else {
             // Acquire the audio graph from the current render thread, shutting it down
+            log::debug!("SinkChange: recover graph from render thread");
+
             let (graph_send, graph_recv) = crossbeam_channel::bounded(1);
             let message = ControlMessage::CloseAndRecycle { sender: graph_send };
             ctrl_msg_send.send(message).unwrap();
@@ -321,6 +330,7 @@ impl AudioContext {
             graph_recv.recv().unwrap()
         };
 
+        log::debug!("SinkChange: closing audio stream");
         backend_manager_guard.close();
 
         // hotswap the backend
@@ -330,10 +340,12 @@ impl AudioContext {
             sink_id,
             render_size_hint: AudioContextRenderSizeCategory::default(), // todo reuse existing setting
         };
+        log::debug!("SinkChange: starting audio stream");
         *backend_manager_guard = io::build_output(options, self.render_thread_init.clone());
 
         // if the previous backend state was suspend, suspend the new one before shipping the graph
         if original_state == AudioContextState::Suspended {
+            log::debug!("SinkChange: suspending audio stream");
             backend_manager_guard.suspend();
         }
 
@@ -352,6 +364,7 @@ impl AudioContext {
         // trigger event when all the work is done
         let _ = self.base.send_event(EventDispatch::sink_change());
 
+        log::debug!("SinkChange: done");
         Ok(())
     }
 
@@ -422,7 +435,15 @@ impl AudioContext {
     /// * The audio device is not available
     /// * For a `BackendSpecificError`
     pub async fn suspend(&self) {
-        // First, pause rendering via a control message
+        // Don't lock the backend manager because we can't hold is across the await point
+        log::debug!("Suspend called");
+
+        if self.state() != AudioContextState::Running {
+            log::debug!("Suspend no-op - context is not running");
+            return;
+        }
+
+        // Pause rendering via a control message
         let (sender, receiver) = oneshot::channel();
         let notify = OneshotNotify::Async(sender);
         self.base
@@ -430,10 +451,14 @@ impl AudioContext {
 
         // Wait for the render thread to have processed the suspend message.
         // The AudioContextState will be updated by the render thread.
+        log::debug!("Suspending audio graph, waiting for signal..");
         receiver.await.unwrap();
 
         // Then ask the audio host to suspend the stream
+        log::debug!("Suspended audio graph. Suspending audio stream..");
         self.backend_manager.lock().unwrap().suspend();
+
+        log::debug!("Suspended audio stream");
     }
 
     /// Resumes the progression of time in an audio context that has previously been
@@ -445,19 +470,34 @@ impl AudioContext {
     ///
     /// * The audio device is not available
     /// * For a `BackendSpecificError`
+    #[allow(clippy::await_holding_lock)] // false positive due to explicit drop
     pub async fn resume(&self) {
-        // First ask the audio host to resume the stream
-        self.backend_manager.lock().unwrap().resume();
+        // Lock the backend manager mutex to avoid concurrent calls
+        log::debug!("Resume called, locking backend manager");
+        let backend_manager_guard = self.backend_manager.lock().unwrap();
+
+        if self.state() != AudioContextState::Suspended {
+            log::debug!("Resume no-op - context is not suspended");
+            return;
+        }
+
+        // Ask the audio host to resume the stream
+        backend_manager_guard.resume();
 
         // Then, ask to resume rendering via a control message
+        log::debug!("Resumed audio stream, waking audio graph");
         let (sender, receiver) = oneshot::channel();
         let notify = OneshotNotify::Async(sender);
         self.base
             .send_control_msg(ControlMessage::Resume { notify });
 
+        // Drop the Mutex guard so we won't hold it across an await
+        drop(backend_manager_guard);
+
         // Wait for the render thread to have processed the resume message
         // The AudioContextState will be updated by the render thread.
         receiver.await.unwrap();
+        log::debug!("Resumed audio graph");
     }
 
     /// Closes the `AudioContext`, releasing the system resources being used.
@@ -469,22 +509,37 @@ impl AudioContext {
     ///
     /// Will panic when this function is called multiple times
     pub async fn close(&self) {
-        // First, stop rendering via a control message
-        let (sender, receiver) = oneshot::channel();
-        let notify = OneshotNotify::Async(sender);
-        self.base.send_control_msg(ControlMessage::Close { notify });
+        // Don't lock the backend manager because we can't hold is across the await point
+        log::debug!("Close called");
 
-        // Wait for the render thread to have processed the suspend message.
-        // The AudioContextState will be updated by the render thread.
-        receiver.await.unwrap();
+        if self.state() == AudioContextState::Closed {
+            log::debug!("Close no-op - context is already closed");
+            return;
+        }
+
+        if self.state() == AudioContextState::Running {
+            // First, stop rendering via a control message
+            let (sender, receiver) = oneshot::channel();
+            let notify = OneshotNotify::Async(sender);
+            self.base.send_control_msg(ControlMessage::Close { notify });
+
+            // Wait for the render thread to have processed the suspend message.
+            // The AudioContextState will be updated by the render thread.
+            log::debug!("Suspending audio graph, waiting for signal..");
+            receiver.await.unwrap();
+        } else {
+            // if the context is not running, change the state manually
+            self.base.set_state(AudioContextState::Closed);
+        }
 
         // Then ask the audio host to close the stream
+        log::debug!("Suspended audio graph. Closing audio stream..");
         self.backend_manager.lock().unwrap().close();
 
         // Stop the AudioRenderCapacity collection thread
         self.render_capacity.stop();
 
-        // TODO stop the event loop <https://github.com/orottier/web-audio-api-rs/issues/421>
+        log::debug!("Closed audio stream");
     }
 
     /// Suspends the progression of time in the audio context.
@@ -502,7 +557,16 @@ impl AudioContext {
     /// * The audio device is not available
     /// * For a `BackendSpecificError`
     pub fn suspend_sync(&self) {
-        // First, pause rendering via a control message
+        // Lock the backend manager mutex to avoid concurrent calls
+        log::debug!("Suspend_sync called, locking backend manager");
+        let backend_manager_guard = self.backend_manager.lock().unwrap();
+
+        if self.state() != AudioContextState::Running {
+            log::debug!("Suspend_sync no-op - context is not running");
+            return;
+        }
+
+        // Pause rendering via a control message
         let (sender, receiver) = crossbeam_channel::bounded(0);
         let notify = OneshotNotify::Sync(sender);
         self.base
@@ -510,10 +574,14 @@ impl AudioContext {
 
         // Wait for the render thread to have processed the suspend message.
         // The AudioContextState will be updated by the render thread.
+        log::debug!("Suspending audio graph, waiting for signal..");
         receiver.recv().ok();
 
         // Then ask the audio host to suspend the stream
-        self.backend_manager.lock().unwrap().suspend();
+        log::debug!("Suspended audio graph. Suspending audio stream..");
+        backend_manager_guard.suspend();
+
+        log::debug!("Suspended audio stream");
     }
 
     /// Resumes the progression of time in an audio context that has previously been
@@ -529,10 +597,20 @@ impl AudioContext {
     /// * The audio device is not available
     /// * For a `BackendSpecificError`
     pub fn resume_sync(&self) {
-        // First ask the audio host to resume the stream
-        self.backend_manager.lock().unwrap().resume();
+        // Lock the backend manager mutex to avoid concurrent calls
+        log::debug!("Resume_sync called, locking backend manager");
+        let backend_manager_guard = self.backend_manager.lock().unwrap();
+
+        if self.state() != AudioContextState::Suspended {
+            log::debug!("Resume no-op - context is not suspended");
+            return;
+        }
+
+        // Ask the audio host to resume the stream
+        backend_manager_guard.resume();
 
         // Then, ask to resume rendering via a control message
+        log::debug!("Resumed audio stream, waking audio graph");
         let (sender, receiver) = crossbeam_channel::bounded(0);
         let notify = OneshotNotify::Sync(sender);
         self.base
@@ -541,6 +619,7 @@ impl AudioContext {
         // Wait for the render thread to have processed the resume message
         // The AudioContextState will be updated by the render thread.
         receiver.recv().ok();
+        log::debug!("Resumed audio graph");
     }
 
     /// Closes the `AudioContext`, releasing the system resources being used.
@@ -555,22 +634,38 @@ impl AudioContext {
     ///
     /// Will panic when this function is called multiple times
     pub fn close_sync(&self) {
-        // First, stop rendering via a control message
-        let (sender, receiver) = crossbeam_channel::bounded(0);
-        let notify = OneshotNotify::Sync(sender);
-        self.base.send_control_msg(ControlMessage::Close { notify });
+        // Lock the backend manager mutex to avoid concurrent calls
+        log::debug!("Close_sync called, locking backend manager");
+        let backend_manager_guard = self.backend_manager.lock().unwrap();
 
-        // Wait for the render thread to have processed the suspend message.
-        // The AudioContextState will be updated by the render thread.
-        receiver.recv().ok();
+        if self.state() == AudioContextState::Closed {
+            log::debug!("Close no-op - context is already closed");
+            return;
+        }
+
+        // First, stop rendering via a control message
+        if self.state() == AudioContextState::Running {
+            let (sender, receiver) = crossbeam_channel::bounded(0);
+            let notify = OneshotNotify::Sync(sender);
+            self.base.send_control_msg(ControlMessage::Close { notify });
+
+            // Wait for the render thread to have processed the suspend message.
+            // The AudioContextState will be updated by the render thread.
+            log::debug!("Suspending audio graph, waiting for signal..");
+            receiver.recv().ok();
+        } else {
+            // if the context is not running, change the state manually
+            self.base.set_state(AudioContextState::Closed);
+        }
 
         // Then ask the audio host to close the stream
-        self.backend_manager.lock().unwrap().close();
+        log::debug!("Suspended audio graph. Closing audio stream..");
+        backend_manager_guard.close();
 
         // Stop the AudioRenderCapacity collection thread
         self.render_capacity.stop();
 
-        // TODO stop the event loop <https://github.com/orottier/web-audio-api-rs/issues/421>
+        log::debug!("Closed audio stream");
     }
 
     /// Creates a [`MediaStreamAudioSourceNode`](node::MediaStreamAudioSourceNode) from a


Test patch:
diff --git a/tests/online.rs b/tests/online.rs
index ae4ab6c7..ad7f1ad9 100644
--- a/tests/online.rs
+++ b/tests/online.rs
@@ -219,8 +219,15 @@ fn test_closed() {
     let context = AudioContext::new(options);
     let node = context.create_gain();
 
-    // close the context, and drop it as well (otherwise the comms channel is kept alive)
+    // Close the context
     context.close_sync();
+    assert_eq!(context.state(), AudioContextState::Closed);
+
+    // Should not be able to resume
+    context.resume_sync();
+    assert_eq!(context.state(), AudioContextState::Closed);
+
+    // Drop the context (otherwise the comms channel is kept alive)
     drop(context);
 
     // allow some time for the render thread to drop
@@ -228,3 +235,63 @@ fn test_closed() {
 
     node.disconnect(); // should not panic
 }
+
+#[test]
+fn test_double_suspend() {
+    let options = AudioContextOptions {
+        sink_id: "none".into(),
+        ..AudioContextOptions::default()
+    };
+    let context = AudioContext::new(options);
+
+    context.suspend_sync();
+    assert_eq!(context.state(), AudioContextState::Suspended);
+    context.suspend_sync();
+    assert_eq!(context.state(), AudioContextState::Suspended);
+    context.resume_sync();
+    assert_eq!(context.state(), AudioContextState::Running);
+}
+
+#[test]
+fn test_double_resume() {
+    let options = AudioContextOptions {
+        sink_id: "none".into(),
+        ..AudioContextOptions::default()
+    };
+    let context = AudioContext::new(options);
+
+    context.suspend_sync();
+    assert_eq!(context.state(), AudioContextState::Suspended);
+    context.resume_sync();
+    assert_eq!(context.state(), AudioContextState::Running);
+    context.resume_sync();
+    assert_eq!(context.state(), AudioContextState::Running);
+}
+
+#[test]
+fn test_double_close() {
+    let options = AudioContextOptions {
+        sink_id: "none".into(),
+        ..AudioContextOptions::default()
+    };
+    let context = AudioContext::new(options);
+
+    context.close_sync();
+    assert_eq!(context.state(), AudioContextState::Closed);
+    context.close_sync();
+    assert_eq!(context.state(), AudioContextState::Closed);
+}
+
+#[test]
+fn test_suspend_then_close() {
+    let options = AudioContextOptions {
+        sink_id: "none".into(),
+        ..AudioContextOptions::default()
+    };
+    let context = AudioContext::new(options);
+
+    context.suspend_sync();
+    assert_eq!(context.state(), AudioContextState::Suspended);
+    context.close_sync();
+    assert_eq!(context.state(), AudioContextState::Closed);
+}