Fix nested ASIO streams causing deadlock on drop

CHANGELOG.md

@@ -37,6 +37,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **ALSA**: Fix a remaining timestamp segfault on 32-bit platforms with a 64-bit kernel `time_t`.
+- **ASIO**: Fix a deadlock when dropping a `Stream` that owns another ASIO `Stream`.
+- **ASIO**: Fix loading a driver while a previous driver was still unloading.
 - **AudioWorklet**: Fix processor construction failures not being reported to `error_callback`.
 - **JACK**: Channel enumeration is capped at the physical system port count again.
 - **WASAPI**: Device enumeration no longer panics if the COM enumerator fails to initialize.

asio-sys/src/bindings/mod.rs

@@ -31,10 +31,8 @@ use self::asio_import as ai;
 /// There should only be one instance of this type at any point in time.
 #[derive(Debug, Default)]
 pub struct Asio {
-    // Keeps track of whether or not a driver is already loaded.
-    //
-    // This is necessary as ASIO only supports one `Driver` at a time.
-    loaded_driver: Mutex<Weak<DriverInner>>,
+    // Guards driver load and teardown. ASIO only supports one driver at a time.
+    loaded_driver: Arc<Mutex<Weak<DriverInner>>>,
 }
 
 /// A handle to a single ASIO driver.
@@ -74,6 +72,9 @@ struct DriverInner {
     // In the case that the driver has been manually destroyed this flag will be set to `true`
     // indicating to the `drop` implementation that there is nothing to be done.
     destroyed: bool,
+    // Shared with the owning `Asio`; locked during teardown so a concurrent `load_driver`
+    // can't start `ASIOInit` before this driver's `ASIOExit` finishes.
+    loaded_driver: Arc<Mutex<Weak<DriverInner>>>,
 }
 
 /// All possible states of an ASIO `Driver` instance.
@@ -449,12 +450,16 @@ impl Asio {
 
         // Check whether or not a driver is already loaded.
         if let Some(inner) = loaded.upgrade() {
-            let driver = Driver { inner };
-            if driver.name() == driver_name {
-                return Ok(driver);
+            let result = if inner.name == driver_name {
+                Ok(Driver { inner })
             } else {
-                return Err(LoadDriverError::DriverAlreadyExists);
-            }
+                Err(LoadDriverError::DriverAlreadyExists)
+            };
+
+            // Release before `result` goes out of scope: if it holds the last handle,
+            // `DriverInner::drop` takes this same lock.
+            drop(loaded);
+            return result;
         }
 
         // Make owned CString to send to load driver
@@ -486,6 +491,7 @@ impl Asio {
                         state,
                         streams,
                         destroyed,
+                        loaded_driver: self.loaded_driver.clone(),
                     });
                     *loaded = Arc::downgrade(&inner);
                     let driver = Driver { inner };
@@ -901,7 +907,16 @@ impl Driver {
     /// Remove the callback with the given ID.
     pub fn remove_callback(&self, rem_id: BufferCallbackId) {
         let mut bc = BUFFER_CALLBACK.lock().unwrap();
-        bc.retain(|&(id, _)| id != rem_id);
+        // `remove` (not `swap_remove`) to preserve the insertion order that
+        // `add_callback` relies on for generating the next ID.
+        let removed = bc
+            .iter()
+            .position(|&(id, _)| id == rem_id)
+            .map(|pos| bc.remove(pos));
+        // the lock must be dropped first, as the removed callback could
+        // be owning another stream, which would result in a deadlock
+        drop(bc);
+        drop(removed);
     }
 
     /// Consumes and destroys the `Driver`, stopping the streams if they are running and releasing
@@ -955,7 +970,16 @@ impl Driver {
     /// Remove the event callback with the given ID.
     pub fn remove_event_callback(&self, rem_id: DriverEventCallbackId) {
         let mut dcb = DRIVER_EVENT_CALLBACKS.lock().unwrap();
-        dcb.retain(|&(id, _)| id != rem_id);
+        // `remove` (not `swap_remove`) to preserve the insertion order that
+        // `add_event_callback` relies on for generating the next ID.
+        let removed = dcb
+            .iter()
+            .position(|&(id, _)| id == rem_id)
+            .map(|pos| dcb.remove(pos));
+        // the lock must be dropped first, as the removed callback could
+        // be owning another stream, which would result in a deadlock
+        drop(dcb);
+        drop(removed);
     }
 }
 
@@ -1015,20 +1039,32 @@ impl DriverInner {
     }
 
     fn destroy_inner(&mut self) -> Result<(), AsioError> {
-        {
+        let result = {
+            // Held so a concurrent `load_driver` can't start ASIOInit before this
+            // driver's ASIOExit finishes.
+            let _loaded_driver_guard = self
+                .loaded_driver
+                .lock()
+                .expect("failed to acquire loaded driver lock");
+
             let mut state = self.lock_state();
-            state.destroy()?;
+            state.destroy()
+        };
 
-            // Clear any existing stream callbacks.
-            if let Ok(mut bcs) = BUFFER_CALLBACK.lock() {
-                bcs.clear();
-            }
-        }
+        // Clear any existing stream callbacks. Take the callbacks out and drop the
+        // lock before dropping them, as a callback could be owning another stream,
+        // which would result in a deadlock. Kept outside the guard above: a callback can
+        // take that lock too.
+        let cleared = BUFFER_CALLBACK
+            .lock()
+            .ok()
+            .map(|mut bcs| std::mem::take(&mut *bcs));
+        drop(cleared);
 
-        // Signal that the driver has been destroyed.
-        self.destroyed = true;
+        // Signal that the driver has been destroyed. Left unset on failure so `Drop` retries.
+        self.destroyed = result.is_ok();
 
-        Ok(())
+        result
     }
 }
 

src/lib.rs

@@ -98,6 +98,12 @@
 //! > please share your issue and use-case with the CPAL team on the GitHub issue tracker for
 //! > consideration.*
 //!
+//! The data callback is real-time code. You must not allocate, lock, block, or do I/O in it, and
+//! you must not call back into CPAL from it. Reentrancy isn't supported and backends differ in how
+//! they fail, up to deadlocking the callback thread. To get data in and out, hand it across a
+//! lock-free ring buffer (the `feedback` example uses `ringbuf`) or share simple state through
+//! atomics.
+//!
 //! In this example, we simply fill the given output buffer with silence.
 //!
 //! ```no_run

src/traits.rs

@@ -250,7 +250,8 @@ pub trait DeviceTrait: PartialEq + Eq + Hash + Debug + Display + Send + Sync {
     ///
     /// * `config` - The stream configuration including sample rate, channels, and buffer size.
     /// * `data_callback` - Called periodically with captured audio data. The callback receives
-    ///   a slice of samples in the format `T` and timing information.
+    ///   a slice of samples in the format `T` and timing information. It is real-time code: see
+    ///   the [crate documentation](crate) for what you must not do in it.
     /// * `error_callback` - Called when a stream error occurs (e.g., device disconnected).
     /// * `timeout` - Time to wait for the backend to initialize the stream. `None` waits
     ///   indefinitely; `Some(duration)` limits how long to wait. Note: not all backends honor
@@ -307,7 +308,8 @@ pub trait DeviceTrait: PartialEq + Eq + Hash + Debug + Display + Send + Sync {
     /// * `data_callback` - Called periodically to fill the output buffer. The callback receives
     ///   a mutable slice of samples in the format `T` to be filled with audio data, along with
     ///   timing information. The slice is pre-filled with silence, so a callback that writes
-    ///   fewer samples than the slice holds leaves the remainder silent rather than stale.
+    ///   fewer samples than the slice holds leaves the remainder silent rather than stale. It is
+    ///   real-time code: see the [crate documentation](crate) for what you must not do in it.
     /// * `error_callback` - Called when a stream error occurs (e.g., device disconnected).
     /// * `timeout` - Time to wait for the backend to initialize the stream. `None` waits
     ///   indefinitely; `Some(duration)` limits how long to wait. Note: not all backends honor
@@ -366,7 +368,8 @@ pub trait DeviceTrait: PartialEq + Eq + Hash + Debug + Display + Send + Sync {
     ///
     /// * `config` - The stream configuration including sample rate, channels, and buffer size.
     /// * `sample_format` - The sample format of the audio data.
-    /// * `data_callback` - Called periodically with captured audio data as a [`Data`] buffer.
+    /// * `data_callback` - Called periodically with captured audio data as a [`Data`] buffer. It
+    ///   is real-time code: see the [crate documentation](crate) for what you must not do in it.
     /// * `error_callback` - Called when a stream error occurs (e.g., device disconnected).
     /// * `timeout` - Time to wait for the backend to initialize the stream. `None` waits
     ///   indefinitely; `Some(duration)` limits how long to wait. Note: not all backends honor
@@ -413,6 +416,7 @@ pub trait DeviceTrait: PartialEq + Eq + Hash + Debug + Display + Send + Sync {
     /// * `data_callback` - Called periodically to fill the output buffer with audio data as
     ///   a mutable [`Data`] buffer. The buffer is pre-filled with silence, so a callback that
     ///   writes fewer samples than the buffer holds leaves the remainder silent rather than stale.
+    ///   It is real-time code: see the [crate documentation](crate) for what you must not do in it.
     /// * `error_callback` - Called when a stream error occurs (e.g., device disconnected).
     /// * `timeout` - Time to wait for the backend to initialize the stream. `None` waits
     ///   indefinitely; `Some(duration)` limits how long to wait. Note: not all backends honor
@@ -453,7 +457,8 @@ pub trait DeviceTrait: PartialEq + Eq + Hash + Debug + Display + Send + Sync {
     /// # Parameters
     ///
     /// * `config` - Channels, sample rate, and buffer size shared by both directions.
-    /// * `data_callback` - Called periodically with captured input and a mutable output buffer.
+    /// * `data_callback` - Called periodically with captured input and a mutable output buffer. It
+    ///   is real-time code: see the [crate documentation](crate) for what you must not do in it.
     /// * `error_callback` - Called when a stream error occurs (e.g., device disconnected).
     /// * `timeout` - Time to wait for the backend to initialize the stream. `None` waits
     ///   indefinitely; `Some(duration)` limits how long to wait. Note: not all backends honor