MaulingMonkey / thindx-xaudio2

Rust bindings for XAudio2
Other
1 stars 0 forks source link

Add callback guards #16

Closed MaulingMonkey closed 1 year ago

MaulingMonkey commented 1 year ago
MaulingMonkey commented 1 year ago

I can't figure out if catch_unwind -> abort is necessary:

If an unwinding operation does encounter an ABI boundary that is not permitted to unwind, the behavior depends on the source of the unwinding (Rust panic or a foreign exception): panic will cause the process to safely abort. https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-unwinding

disagrees with:

It is currently undefined behavior to unwind from Rust code into foreign code https://doc.rust-lang.org/std/panic/fn.catch_unwind.html

I think this is just because the C-unwind RFC hasn't stabilized yet? https://github.com/rust-lang/rust/issues/74990 Do I still need to catch_unwind to abort on inconvenient panics in a library context where I can't guarantee panic = "abort"?

MaulingMonkey commented 1 year ago

IXAudio2 / IXAudio2Voice being !Send !Sync means I can't DestroyVoice from the callbacks anyways

shinmao commented 1 year ago

Hi, I think the potential risk of panic still exists in the code of xaudio2_thread_guard(). https://github.com/MaulingMonkey/thindx-xaudio2/blob/a9239a82bf2715c65566087e1e690170dd021199/crates/thindx-xaudio2/src/util/_util.rs#L19 Based on the documentation of eprintln!(), this function will trigger panic when writing to std::err failed. I think we should use something that could return Result to avoid panic.

Maybe we can directly invoke this function https://doc.rust-lang.org/std/io/struct.StderrLock.html#method.write?

How do you think about it?

MaulingMonkey commented 1 year ago

Good catch!