Merge pull request #499 from orottier/bugfix/double-suspend-hangs

Fix hanging UI thread for certain AudioContext::suspend/stop actions

Author: orottier

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);

src/context/online.rs

@@ -279,21 +279,26 @@ 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
         }
 
         if !is_valid_sink_id(&sink_id) {
             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,13 +308,17 @@ 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,
                 _ => unreachable!(),
             }
         } 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,18 +435,30 @@ 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
             .send_control_msg(ControlMessage::Suspend { 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();
 
         // 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,18 +557,31 @@ 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
             .send_control_msg(ControlMessage::Suspend { 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();
 
         // 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

tests/online.rs

@@ -219,12 +219,79 @@ 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
     std::thread::sleep(Duration::from_millis(10));
 
     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);
+}