fix: remove unsoundness with impl Send+Sync for OutputChannel

[danya02]

Previously the code had this:

// SAFETY: This is necessary because [OutputStream] does not implement [Send],
// due to some limitation with Android's Audio API.
// I'm pretty sure nobody will use lowfi with android, so this is safe.
unsafe impl Send for Player {}

// SAFETY: See implementation for [Send].
unsafe impl Sync for Player {}

Now, there may not be a good reason why OutputStream (and particularly cpal::Stream) are not Send (see RustAudio/cpal#793: they did this in anticipation of Android support which didn't exist at the time).
However, it still feels wrong to use unsafe for this, as we're essentially lying to the type system.

So, if there isn't anything preventing you from building it on Android, and it still has the thread limitation, then (with the Tokio multithread executor) it's going to be very easy to cause mass hysteria and thread model errors.

I identified two solutions:

1. (simpler, but worse) Leak the OutputChannel at the point of creation. We don't need access to it as such, because all interaction can be done with the OutputChannelHandle. By leaking it, we make it live forever, which is probably acceptable (as we only ever intend to spawn a single Player per application instance), but it means that the on-Drop cleanup is now left up to the OS. This works fine on Linux and Windows, but it might be a problem on others. Regardless, leaking memory is a Safe Rust operation, so this should not cause soundness bugs on any platform if the underlying library is sound.
2. (more complicated, but better) Spawn a separate thread to hold the OutputChannel for us. We will talk to the thread with a std::sync::mpsc::channel (which is Sendable, so it lives in the Player). When the Player is dropped, the channel is too, and that signals the thread to exit, which runs any Drop cleanup that may be needed.

Initially I implemented the first approach, but then I realized that the second approach works and is even safer (as the drop-time code is now guaranteed to run).
Both approaches have been tested to work on x86_64-unknown-linux-gnu and x86_64-pc-windows-msvc.

---

In addition, I made it so that the curious code you're using to temporarily swap out the stderr file stream is now only compiled in on Linux: it only makes sense to use it there, because it refers to the literal path of /dev/null.
I'm not a big fan of that either, and I would look for a solution that doesn't use unsafe in your code.
The two freopen calls are the only other place where unsafe is used, so if those were removed then you could make the entire project #![forbid(unsafe_code)].

[talwat]

I'm conflicted on this, on the one hand, I see the appeal of not having extra unsafe code. On the other hand, the lack of Sync & Send seem to only be present because of android, with no other real problems on other platforms. If there's a way to prohibit explicitly compilation on android, I'd prefer it. Using an extra thread is probably the proper solution, but it introduces a lot more complexity and it goes against how most of the other state is handled in lowfi.

As for redirecting alsa, making that compile only on Linux is probably the right call. I tried searching for a safe API to freopen, but found nothing. It's preferable to having alsa output mess up the terminal though. If there's a safe API introduced, I'll definitely switch to it.

[danya02]

If there's a way to prohibit explicitly compilation on android, I'd prefer it

You might be able to do:

#[cfg(target_os = "android")]
compile_error!("Android Audio API not supported due to threading shenanigans")

but I don't know if the target_os field is always set to "android" on the problematic platforms. Anyway, some people build their Rust projects on Android phones with Termux, and they might like some lofi music while they're doing it, so if I were doing it I'd consider supporting that (if it's not too difficult of course, and this isn't very).

it goes against how most of the other state is handled in lowfi

Well, here we have essentially a system resource (like a file descriptor or socket or something), and not just a bundle of data, so I argue that it's not a good idea to carry that around with the rest of your Client's business-logic (like the track info or the volume settings or the HTTP client).

Maybe it would be better to hide all the threading mess into a new struct, which would spawn the thread on creation and do the rest of the setup, and then your Client just needs to hold that struct inside it? That way you still get the "benefit" of having the resource live in a separate thread, and also not having to think about it.

(I'm guessing you aren't interested in my original leaking idea?)

[talwat]

Update on this after a bit, after ece88de, I was able to get rid of the unsafe implementation for Sync, which most people at cpal agreed was likely out of the picture anyway from the issues I've read. I did this by just holding the OutputStream in the main thread, rather than attaching it to the player struct.

Maybe it would be better to hide all the threading mess into a new struct, which would spawn the thread on creation and do the rest of the setup, and then your Client just needs to hold that struct inside it? That way you still get the "benefit" of having the resource live in a separate thread, and also not having to think about it.

I think with the changes I made, this could become more practical, but it would still carry the overhead of having a whole extra thread assigned to do nothing but wait.

(I'm guessing you aren't interested in my original leaking idea?)

I'm not entirely sure, but from what I know, the leak would have to be cleaned up by the operating system. Creating an intentional memory leak doesn't really seem all that much better than lying to rust about a trait implementing Send, because of a potentially arbitrary limitation by cpal.

Leaking does avoid unsafe code entirely and simply however, so I'd still like to keep it for consideration.

Anyway, let me know what you think.

[danya02]

I've taken a look, and generally speaking I agree with your changes, so I'm closing this PR.

I still would like to move the logic for swapping around the file descriptors away from your application code. Perhaps I'll write a crate to do it.

---

To make sure that your crate doesn't get unsafely built for Android, I would still add a compile-error for it. I've checked, and #[cfg(target_os = "android")] does work for aarch64-linux-android, which is the target of my smartphone. So, if you add the lines in #50 (comment) next to your SendableOutputStream, then that should prevent it from being built on Android.

You can try that it works by creating a new project and doing:

fn main() {
    #[cfg(target_os="android")]
    compile_error!("oh no, smartphones!");
    println!("Hello, world!");
}

This should build and run on your regular computer but fail for that aarch64-linux-android target.

[danya02]

I created https://crates.io/crates/shaddup, and also found https://crates.io/crates/gag which does the same thing in a different way. Consider using one of those.

I haven't looked into exactly how gag works. I know my shaddup crate copies the currently open file descriptor around, and does not try opening it from scratch. This is better than your approach, where you open the /dev/tty (as the output could have been redirected), though for the use-case of a TUI app it doesn't matter much.