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 implemented. (edit: forget this part)

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.

[edwloef]

Like I noted in my other message, dropping a stream within the callback is already an obvious logic error, since it violates realtime guarantees. I'd opt for maybe an assert to catch that case in an as-obvious-as-possible way, until the new API is here.

[roderickvd]

OK, can use your inputs here. Here's my train of thought:

With @LastExceed's reproducible example we've established that dropping a stream within another stream causes a deadlock, and that 6f98225 fixes it.

We all agree that it's crufty.

@edwloef's point about RT-safety is correct: dropping a stream isn't RT-safe, nor is allocating a Vec. The latter we could mostly avoid: reserve capacity for one slot up front, since I'd guess a single nested stream is the common case.

What I'm then wondering is:

1. Should we forbid tearing down streams within streams?
2. Don't we already, by stating that callbacks should be RT-safe?
3. Should we allow nested streams at all?

What we allow or don't allow by contract seems pivotal. If we allow all this, this class of reentrancy cases we need to prepare for. The cruft would be necessary.

If we don't allow this, beyond documenting and asserting that, we could keep the reentrancy check but leak the entry instead of hanging. This would skip the removal, but leave a callback that keeps firing until the driver eventually tears down.

I don't see how #367 would contribute to a more architecturally sound solution, but implementing duplex support for ASIO (on top of #1229) would. It wouldn't fix reentrancy with nested streams, but it wouldn't need them to pair an input with an output stream, which again is an assumption of mine to be the common case.

Then again, pairing multiple streams doesn't require nesting in the first place: they could be run side-by-side.

But before forbidding nested streams by contract, a nagging thought: this class of reentrancy is likely ASIO-only. Forbidding seems a bigger hammer than the bug calls for.

So... keep 6f98225 or what do you guys think? @edwloef what is your need for nesting streams?

[LastExceed]

Don't we already, by stating that callbacks should be RT-safe?

I'd say yes, and that it renders the rest of the conversion obsolete

I don't see how #367 would contribute to a more architecturally sound solution

Forget that part, I got a few things mixed up in my head

this class of reentrancy is likely ASIO-only

with the duplex API obsoleting the input+output usecase, I think we should just remove the ability to have multiple ASIO streams full stop, as it is a massive hack that misrepresents the device's capabilities.

So... keep 6f98225 or what do you guys think?

drop it, it will soon be obsolete anyway. Also I think we're massively overthinking this lol

what is your need for nesting streams?

on Discord they said this:

I, uh
am too lazy to hold two streams in app state
so I have the output stream own the input stream handle, so they both stop when I drop the output stream handle

There is a good chance that there isn't any reasonable use case for this

[edwloef]

with the duplex API obsoleting the input+output usecase

That's not entirely true, for example when you want the input and output to be different devices. Unless the duplex API has recently changed to include that use-case?

[LastExceed]

That scenario is already unsupported even right now, as the ASIO SDK can only load 1 driver at a time.

I guess I should have specified that I specifically mean removing the ability of having multiple streams on the same ASIO device, we just so happen to already not support multiple ASIO devices (#1299 would fix that though).

[roderickvd]

How would #1299 respond to nested streams?

[LastExceed]

Nesting a stream from a different device is trivial, as devices are completely independent of each other. There is no global state.

Nesting a stream from the same device is impossible, as #1299 only allows 1 stream per device

[roderickvd]

The different perspectives seem to converge in the direction to drop 6f98225 and replace it with one that more clearly documents that callbacks should be RT-safe and non-reentrant.

The other fixes can then remain as they cost little.

Let me know if there are other views.

[roderickvd]

Rebased, dropped 6f98225 and force-pushed. Tearing down an ASIO stream from inside another ASIO's stream will now deadlock again, but @edwloef's actual use case (output stream owning the input handle, both dropped from app code) remains fixed by a0601d3.

Instead, I added e134fcc stating the requirement that the callback needs to be RT-safe and non-reentrant, plus another round of fixes for clearing stream callbacks in 31a500d and 643ca9d.

@LastExceed could you do a final round of testing?