refactor: fix clippy warnings, replace Arc<Mutex> with Rc<RefCell> in CoreAudio #998
Conversation
- Replace `format!` with Rust 1.58+ inline format strings - Use `Rc<RefCell<...>>` instead of `Arc<Mutex<...>>` for CoreAudio streams - Update CoreAudio disconnect listener and stream methods to use `Rc<RefCell<...>>` - Fix CoreAudio error code comparisons and comments
|
@wgibbs-rs @tomoyanonymous as recent CoreAudio contributors, would you help to review? |
tomoyanonymous
left a comment
tomoyanonymous
left a comment
There was a problem hiding this comment.
This is an additional suggetion but about the refactoring related to this, in E and D in build_input/output_stream_raw and add_disconnect_listener, Send is not necessary for them.
This is a very interesting point. While correct for CoreAudio, I believe that other backends (WASAPI, ALSA) do spawn threads where callbacks run, so the trait's |
|
Ah, I missed that the constraint came from |
|
@roderickvd This looks great to me. I reviewed what you created for CoreAudio on my own device, and it worked first try. You fixed the mutability issues in StreamTrait, which is perfect, as well as everything else. Previously, I tried working with multi-threading and it failed, and I was confused with the use of an Arc<Mutex<>>, but now that there's explicitly no-multithreading, I think that should be included in the documentation. Let me look and see if there's a place to put that in the docs.rs |
|
Never mind, I was mistaken on documentation. This is all internal, so documentation isn't necessary. Apart from that, this looks great and runs on my MacBook Air M1. |
Remove changelog entry for CoreAudio Arc<Mutex> to Rc<RefCell> refactor as it's an internal implementation detail that doesn't affect public API or user-observable behavior.
|
Thanks for the quick reviews. I removed the changelog entry, as it the change isn't user-facing. Merging now. |
… CoreAudio (RustAudio#998) - Replace `format!` with Rust 1.58+ inline format strings - Use `Rc<RefCell<...>>` instead of `Arc<Mutex<...>>` for CoreAudio streams - Update CoreAudio disconnect listener and stream methods to use `Rc<RefCell<...>>` - Fix CoreAudio error code comparisons and comments
3 participants
Fixex standard clippy warnings, as well as a more impactful change in CoreAudio (macOS): to resolve the
clippy::arc-with-non-send-syncwarning, it replacesArc<Mutex<StreamInner>>withRc<RefCell<StreamInner>>.CoreAudio changes (review requested)
The main change replaces
Arc<Mutex<StreamInner>>withRc<RefCell<StreamInner>>in the macOS CoreAudio implementation. Rationale:StreamInnercontainsAudioUnitandAudioObjectPropertyListenerwhich are notSend/SyncArc<Mutex<>>provided no thread-safety benefits since the inner types aren't thread-safeRc<RefCell<>>is more appropriate for single-threaded shared ownershipThis changes fundamental synchronization primitives in the CoreAudio backend. While the change is theoretically sound and all tests pass, extra scrutiny would be appreciated to ensure that we are not missing some edge cases.