Fix nested ASIO streams causing deadlock on drop

[LastExceed]

Reported by @edwloef (on Discord):

It seems #1297 may have introduced a deadlock. When one stream's callback owns another stream, and that stream is dropped, they try to re-entrantly lock the same lock in the Drop impl and everything dies.

A curious configuration, but alas. Fixing it was a simple as changing the drop order.

[roderickvd]

It's great that you and @edwloef showed this works. Reviewing it though, I see that swapping retain for swap_remove breaks the ordering that add_callback relies on for IDs, so two streams can end up sharing an ID after a removal.

Going through the stream lifecycle, when one callback holding a nested stream, I think there's more issues:

• destroy_inner was clearing callbacks in place while still holding the lock
• a callback dropping a nested stream mid-run could still deadlock in buffer_switch_time_info
• load_driver could start ASIOInit while an old driver's ASIOExit was still running

I'd appreciate your local testing on this.

[edwloef]

6f98225 looks like it introduces a fair bit of non-local complexity for questionable gain: dropping a Stream is realtime-unsafe (I assume?), and should therefore not be happening from within a data callback anyways. Additionally, by pushing into a Vec, avoiding the deadlock is realtime-unsafe as well. I can't test whether it works since I don't have access to a Windows computer, but from an outside POV it seems not worth it to me. Avoiding the deadlock is great and all, but this seems like a situation where failing loudly would be appropriate.

[roderickvd]

Yeah I wouldn't mind removing it either. Let me know if you or @LastExceed not running into that when you tear down mid-callback.

[LastExceed]

What exactly do you want me to test?

[roderickvd]

What exactly do you want me to test?

6f98225: try to drop a nested stream from the parent’s callback. Does it deadlock without that commit?

[LastExceed]

Here is what I tried:

use std::thread;
use cpal::traits::{DeviceTrait, HostTrait, StreamTrait};

fn main() {
    let host = cpal::host_from_id(cpal::HostId::Asio).unwrap();
    let device = host.devices().unwrap().nth(6).unwrap();
    
    let config = device.default_output_config().unwrap().into();
    let error_callback = |error| panic!("{error}");
    
    let mut count_inner = 0;
    let data_callback_inner = move |data: &mut [i32], _: &_| {
        count_inner += 1;
        println!("inner {count_inner}");
        data.fill(0);
    };
    
    println!("start inner");
    let inner_stream = device.build_output_stream(config, data_callback_inner, error_callback, None).unwrap();
    inner_stream.start();
    let mut option = Some(inner_stream);
    
    let mut count_outer = 0;
    let data_callback_outer = move |data: &mut [i32], _: &_| {
        count_outer += 1;
        println!("outer {count_outer}");
        data.fill(0);
        
        if count_outer == 10 {
            drop(option.take());
        }
    };
    
    let outer_stream = device.build_output_stream(config, data_callback_outer, error_callback, None).unwrap();
    
    println!("start outer");
    outer_stream.start();
    
    thread::park();
}

before 6f98225 it deadlocks. after 6f98225 it works fine.

I'll take a look at the code in a sec

[LastExceed]

Ok, I agree that 6f98225 smells.

But I also think that this is kind of an inevitable consequence of trying to support concurrent streams on a backend that doesn't natively do so. This in turn is a necessary workaround for cpal's API currently being unable to properly represent ASIO's stream model (which is basically a single duplex stream per device, with an arbitrary channel constellation, and a sequential instead of interleaved data layout), and something that we should consider removing outright once we got #367 and #1301 (edit: actually forget this one) implemented.

So, if we treat this as just a temporary hack that will become obsolete with the next major release anyway, then I'd vote for keeping the commit, but it is to stay permanently, then I'd vote against it.