Make accidentally dropping OutputStream impossible

[yara-blue]

This adds a drop bomb to OutputStream. Dropping a OutputStream will panic (unless we are dropping as part of unwinding). Technically OutputStream is now a run time enforced linear type.

To get rid of a OutputStream you now have to call OutputStream::close.

For a (worse in my opinion) alternative see: #747

[roderickvd]

For a (worse in my opinion) alternative see: #747

I agree that maintaining our own counter sucks, like in #747. But so is pulling in another dependency, kinda... it's late, but could we use ManuallyDrop from std instead?

Or something like this? I'm actually thinking: isn't Arc designed precisely for this? But I thought we had already wrapped it in an Arc before. I need to go to bed 😄 but here's a rough draft with a wrapper type:

use std::sync::Arc;

// New internal type to wrap the actual stream
struct StreamHandle {
    stream: cpal::Stream,
}

pub struct OutputStream {
    mixer: Mixer,
    _stream: Arc<StreamHandle>,
}

impl OutputStream {  
    pub fn mixer(&self) -> &Mixer {
        &self.mixer
    }
    
    pub fn connect_sink(&self) -> Sink {
        Sink::connect_new(self.mixer())
    }
}

Mixer:

pub struct Mixer {
    _stream: Arc<StreamHandle>,
}

impl Mixer {
    fn new(stream: Arc<StreamHandle>) -> Self {
        Self {
            _stream: stream,
        }
    }

    fn get_stream_ref(&self) -> Arc<StreamHandle> {
        self._stream.clone()
    }
}

Sink:

pub struct Sink {
    _stream: Arc<StreamHandle>,
}

impl Sink {
    pub fn connect_new(mixer: &Mixer) -> Sink {
        let stream_ref = mixer.get_stream_ref();
        
        Sink {
            _stream: stream_ref,
        }
    }
}

[yara-blue]

I agree that maintaining our own counter sucks, like in #747. But so is pulling in another dependency, kinda... it's late, but could we use ManuallyDrop from std instead?

Manually drop does something else, the drop bomb is crate is only a few lines so we can copy those over instead. The author is highly trusted and it made the code clean so I just added it as dependency.

Or something like this? I'm actually thinking: isn't Arc designed precisely for this? But I thought we had already wrapped it in an Arc before. I need to go to bed 😄 but here's a rough draft with a wrapper type:

If only we could, unfortunately cpal::Stream is !Send. If we where to wrap it in an Arc our mixer's could no longer travel between threads. That is why #747 spawns a thread just to create a cpal::Stream in and keep it there.

Another issue is that its possible to drop all sinks and mixers and there should still be audio playing. Though that could probably be helped by adding a "counter" to the mixer source instead of the mixer handle....

[roderickvd]

Argh, what a pain. Well, if this is the lesser of evils, so be it.

[yara-blue]

Argh, what a pain. Well, if this is the lesser of evils, so be it.

We could get #747 to work "perfectly" by moving the counter to the source. But we would have to introduce extra logic to deal with stuff like endless queue's. (We would need to track if the queue's handle has dropped and then (when the queue is empty) lower the counter).

It would still be kinda ugly due to the thread that is simply there to store the cpal::Stream :disappointed:.

Still in favor of this PR, though I really don't like making every downstream user add a custom drop implementations to whatever they are storing our OutputStream in.....

[roderickvd]

Still in favor of this PR, though I really don't like making every downstream user add a custom drop implementations to whatever they are storing our OutputStream in.....

The other option is of course to do nothing, and have users retain the stream in their struct.

[yara-blue]

The other option is of course to do nothing, and have users retain the stream in their struct.

maybe there is a middle ground: We emit a warning on drop unless close is called. Its not as disrupting as a panic but it could still help out new users.

[roderickvd]

maybe there is a middle ground: We emit a warning on drop unless close is called. Its not as disrupting as a panic but it could still help out new users.

In the case of the examples as they are today, would that mean that a warning is emitted for most of them? I can imagine that a user wouldn't care when it just works with one file and exits, but a warning may surprise them.

[yara-blue]

In the case of the examples as they are today, would that mean that a warning is emitted for most of them?

No not if we call .close() on the stream before the end of the function. Instead of a drop bomb we have a drop shout. We can emit the shout at the debug log level for those using tracing or log, that its pretty non-disruptive.

[roderickvd]

OK, let's do that.

[yara-blue]

OK, let's do that.

I'll get on that tomorrow and then afterwards see if I can prep the next release

[PetrGlad]

Could it be an option to provide drop diagnostics instead? Drop may show the stack in debug mode, in lower log level settings this could be only a message mentioning the operation.

[PetrGlad]

Another alternative - to recommend keeping the output stream in a static cell, maybe?

[yara-blue]

Could it be an option to provide drop diagnostics instead?

this is the plan now, we will print on drop unless close() was called first.

Another alternative - to recommend keeping the output stream in a static cell, maybe?

Dropping the stream before it should close is mostly an issue about not reading the docs. If you read those you will see you should not drop the stream before you are done playing sound.

I do not like users having to read the docs in detail (and remember it all!). Rodio should be really easy to use. No sound (without any warning) because you dropped a variable early makes Rodio hard to use. This PR adds a crash which is impossible to miss. The proposal is to make it a warning/diagnostic.

[yara-blue]

lets remove close and the drop bomb, instead we always print something when the outputstream is dropped. We add a function to outputstream which one can use to disable the print. And we call that out from the print msg.

[yara-blue]

closed in favor of https://github.com/RustAudio/rodio/pull/761