From 02808ed6b4beb9472d4087f66a08961a81ebe90b Mon Sep 17 00:00:00 2001 From: evdokimovs <49490279+evdokimovs@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:13:19 -0300 Subject: [PATCH] Get rid of `static mut` usage (#193) Additionally: - temporary disable `wasm-opt` (rustwasm/wasm-pack#1441) Co-authored-by: alexlapa Co-authored-by: Kai Ren --- Cargo.toml | 2 ++ crates/medea-macro/src/dart_bridge.rs | 1 + src/api/dart/api/local_media_track.rs | 9 ++++-- src/api/dart/api/mod.rs | 14 ++++----- src/api/dart/api/remote_media_track.rs | 9 ++++-- src/api/dart/err.rs | 10 +++++-- src/platform/dart/executor/mod.rs | 21 +++++++++----- src/platform/dart/mod.rs | 40 +++++++++++++++----------- src/platform/dart/utils/callback.rs | 21 +++++++++----- src/platform/dart/utils/dart_future.rs | 22 +++++++++----- src/platform/dart/utils/string.rs | 24 +++++++++------- 11 files changed, 109 insertions(+), 64 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2fe1f756..1467fbdb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,8 @@ readme = "README.md" keywords = ["medea", "jason", "webrtc", "client", "browser"] categories = ["multimedia", "api-bindings", "web-programming", "wasm"] include = ["/src/", "/build.rs", "/CHANGELOG.md", "/LICENSE.md"] +# TODO: Remove when https://github.com/rustwasm/wasm-pack/issues/1441 will be resolved. +wasm-opt = false [workspace] members = [ diff --git a/crates/medea-macro/src/dart_bridge.rs b/crates/medea-macro/src/dart_bridge.rs index e776374f..3b2cc97a 100644 --- a/crates/medea-macro/src/dart_bridge.rs +++ b/crates/medea-macro/src/dart_bridge.rs @@ -11,6 +11,7 @@ use syn::{parse_quote, punctuated::Punctuated, spanned::Spanned as _, token}; #[cfg(feature = "dart-codegen")] use crate::dart_codegen::{DartCodegen, FnRegistrationBuilder}; +// TODO: Refactor to get rid of `static mut` in this macro. /// Expands `#[dart_bridge]` attribute placed on a Rust module declaration. pub(crate) fn expand( args: TokenStream, diff --git a/src/api/dart/api/local_media_track.rs b/src/api/dart/api/local_media_track.rs index a5466755..95a0078b 100644 --- a/src/api/dart/api/local_media_track.rs +++ b/src/api/dart/api/local_media_track.rs @@ -40,9 +40,12 @@ impl LocalMediaTrack { #[frb(sync)] #[must_use] pub fn get_track(&self) -> DartOpaque { - DartOpaque::new(self.0.get_track().handle() as _, unsafe { - DART_HANDLER_PORT.unwrap() - }) + DartOpaque::new( + self.0.get_track().handle() as _, + DART_HANDLER_PORT + .get() + .expect("`DART_HANDLER_PORT` must be initialized"), + ) } /// Returns a [`MediaKind::Audio`] if the provided [`LocalMediaTrack`] diff --git a/src/api/dart/api/mod.rs b/src/api/dart/api/mod.rs index 950190b0..63fbd767 100644 --- a/src/api/dart/api/mod.rs +++ b/src/api/dart/api/mod.rs @@ -41,7 +41,7 @@ pub mod remote_media_track; pub mod room; pub mod room_close_reason; -use std::ptr; +use std::{cell::Cell, ptr}; use flutter_rust_bridge::{frb, DartOpaque}; @@ -63,8 +63,10 @@ pub use self::{ room::RoomHandle, room_close_reason::RoomCloseReason, }; -/// Used to create [`DartOpaque`]s on the Rust side. -pub static mut DART_HANDLER_PORT: Option = None; +thread_local! { + /// Used to create [`DartOpaque`]s on the Rust side. + pub static DART_HANDLER_PORT: Cell> = Cell::default(); +} /// Rust structure having wrapper class in Dart. /// @@ -381,9 +383,5 @@ pub fn on_panic(cb: DartOpaque) { #[frb(sync)] #[must_use] pub fn set_dart_opaque_message_port(dart_handler_port: i64) { - // TODO: Refactor to get rid of `static mut`. - #[expect(static_mut_refs, reason = "needs refactoring")] - unsafe { - DART_HANDLER_PORT.replace(dart_handler_port); - } + DART_HANDLER_PORT.set(Some(dart_handler_port)); } diff --git a/src/api/dart/api/remote_media_track.rs b/src/api/dart/api/remote_media_track.rs index 61750dee..d8497aca 100644 --- a/src/api/dart/api/remote_media_track.rs +++ b/src/api/dart/api/remote_media_track.rs @@ -34,9 +34,12 @@ impl RemoteMediaTrack { #[frb(sync)] #[must_use] pub fn get_track(&self) -> DartOpaque { - DartOpaque::new(self.0.get_track().handle() as _, unsafe { - DART_HANDLER_PORT.unwrap() - }) + DartOpaque::new( + self.0.get_track().handle() as _, + DART_HANDLER_PORT + .get() + .expect("`DART_HANDLER_PORT` must be initialized"), + ) } /// Sets callback to invoke once this [`RemoteMediaTrack`] is muted. diff --git a/src/api/dart/err.rs b/src/api/dart/err.rs index 749926eb..1cdcc76f 100644 --- a/src/api/dart/err.rs +++ b/src/api/dart/err.rs @@ -135,12 +135,16 @@ impl DartError { } } -#[expect(clippy::fallible_impl_from, reason = "FFI error is unexpected")] impl From for DartOpaque { fn from(val: DartError) -> Self { let boxed = unsafe { Box::from_raw(val.0.as_ptr()) }; - #[expect(clippy::unwrap_used, reason = "FFI error is unexpected")] - Self::new((*boxed).cast(), unsafe { DART_HANDLER_PORT.unwrap() }) + #[expect(clippy::expect_used, reason = "intended behavior")] + Self::new( + (*boxed).cast(), + DART_HANDLER_PORT + .get() + .expect("`DART_HANDLER_PORT` must be initialized"), + ) } } diff --git a/src/platform/dart/executor/mod.rs b/src/platform/dart/executor/mod.rs index b53ad3f8..45c1b27f 100644 --- a/src/platform/dart/executor/mod.rs +++ b/src/platform/dart/executor/mod.rs @@ -2,7 +2,12 @@ mod task; -use std::{future::Future, ptr, rc::Rc}; +use std::{ + future::Future, + ptr, + rc::Rc, + sync::{atomic, atomic::AtomicI64}, +}; use dart_sys::{ Dart_CObject, Dart_CObject_Type_Dart_CObject_kInt64, Dart_Port, @@ -18,12 +23,15 @@ pub fn spawn(fut: impl Future + 'static) { Task::spawn(Box::pin(fut)); } -/// A [`Dart_Port`] used to send [`Task`]'s poll commands so Dart will poll Rust +/// Atomic variant of a [`Dart_Port`]. +type AtomicDartPort = AtomicI64; + +/// [`Dart_Port`] used to send [`Task`]'s poll commands so Dart will poll Rust /// [`Future`]s. /// /// Must be initialized with the [`rust_executor_init()`] function during FFI /// initialization. -static mut WAKE_PORT: Option = None; +static WAKE_PORT: AtomicDartPort = AtomicI64::new(0); /// Initializes Dart-driven async [`Task`] executor. /// @@ -35,9 +43,7 @@ static mut WAKE_PORT: Option = None; /// Must ONLY be called by Dart during FFI initialization. #[no_mangle] pub unsafe extern "C" fn rust_executor_init(wake_port: Dart_Port) { - unsafe { - WAKE_PORT = Some(wake_port); - } + WAKE_PORT.store(wake_port, atomic::Ordering::Release); } /// Polls the provided [`Task`]. @@ -58,7 +64,8 @@ pub unsafe extern "C" fn rust_executor_poll_task(task: ptr::NonNull) { /// [`WAKE_PORT`]. When received, Dart must poll it by calling the /// [`rust_executor_poll_task()`] function. fn task_wake(task: Rc) { - let wake_port = unsafe { WAKE_PORT }.unwrap(); + let wake_port = WAKE_PORT.load(atomic::Ordering::Acquire); + debug_assert!(wake_port > 0, "`WAKE_PORT` address must be initialized"); let task = Rc::into_raw(task); let mut task_addr = Dart_CObject { diff --git a/src/platform/dart/mod.rs b/src/platform/dart/mod.rs index 181ea359..e1e3f5e1 100644 --- a/src/platform/dart/mod.rs +++ b/src/platform/dart/mod.rs @@ -30,7 +30,10 @@ pub mod transceiver; pub mod transport; pub mod utils; -use std::panic; +use std::{ + cell::{LazyCell, RefCell}, + panic, +}; use libc::c_void; @@ -66,32 +69,31 @@ pub unsafe extern "C" fn init_jason_dart_api_dl(data: *mut c_void) -> isize { /// Dart's functions. pub fn set_panic_hook() { panic::set_hook(Box::new(|bt| { - // TODO: Refactor to get rid of `static mut`. - #[expect(static_mut_refs, reason = "needs refactoring")] - if let Some(f) = unsafe { PANIC_FN.as_ref() } { - f.call1(format!("{bt}")); - } + PANIC_FN.with_borrow(|f| { + if let Some(f) = f { + f.call1(format!("{bt}")); + } + }); })); } -/// [`Function`] being called whenever Rust code [`panic`]s. -static mut PANIC_FN: Option> = None; +thread_local! { + /// [`Function`] being called whenever Rust code [`panic`]s. + static PANIC_FN: RefCell>> = RefCell::default(); +} /// Sets the provided [`Function`] as a callback to be called whenever Rust code /// [`panic`]s. /// /// [`panic`]: panic! pub fn set_panic_callback(cb: Function) { - unsafe { - PANIC_FN = Some(cb); - } + PANIC_FN.set(Some(cb)); } #[cfg(target_os = "android")] /// Initializes [`android_logger`] as the default application logger with filter /// level set to [`log::LevelFilter::Debug`]. pub fn init_logger() { - // TODO: `android_logger::init_once()` should be called only once. android_logger::init_once( android_logger::Config::default() .with_max_level(log::LevelFilter::Debug), @@ -107,8 +109,14 @@ pub fn init_logger() { /// Initializes [`simple_logger`] as the default application logger with filter /// level set to [`log::LevelFilter::Debug`]. pub fn init_logger() { - // TODO: Should be called only once. - _ = simple_logger::SimpleLogger::new() - .with_level(log::LevelFilter::Debug) - .init(); + thread_local! { + /// [`LazyCell`] ensuring that a [`simple_logger`] is initialized only + /// once. + static INITIALIZED: LazyCell<()> = LazyCell::new(|| { + _ = simple_logger::SimpleLogger::new() + .with_level(log::LevelFilter::Debug) + .init(); + }); + } + INITIALIZED.with(|i| **i); } diff --git a/src/platform/dart/utils/callback.rs b/src/platform/dart/utils/callback.rs index ac41f34c..fd0982ee 100644 --- a/src/platform/dart/utils/callback.rs +++ b/src/platform/dart/utils/callback.rs @@ -220,6 +220,8 @@ extern "C" fn callback_finalizer(_: *mut c_void, cb: *mut c_void) { pub mod tests { #![expect(clippy::missing_safety_doc, reason = "only for testing")] + use std::cell::RefCell; + use dart_sys::Dart_Handle; use crate::api::DartValueArg; @@ -272,24 +274,29 @@ pub mod tests { type TestCallbackHandleFunction = extern "C" fn(Dart_Handle); - static mut TEST_CALLBACK_HANDLE_FUNCTION: Option< - TestCallbackHandleFunction, - > = None; + thread_local! { + static TEST_CALLBACK_HANDLE_FUNCTION: RefCell> = RefCell::default(); + } #[no_mangle] pub unsafe extern "C" fn register__test__test_callback_handle_function( f: TestCallbackHandleFunction, ) { - unsafe { - TEST_CALLBACK_HANDLE_FUNCTION = Some(f); - } + TEST_CALLBACK_HANDLE_FUNCTION.set(Some(f)); } + #[expect(clippy::expect_used, reason = "intended behavior")] #[no_mangle] pub unsafe extern "C" fn test_callback_listener_dart_handle() -> Dart_Handle { Callback::from_once(move |val: Dart_Handle| { - (unsafe { TEST_CALLBACK_HANDLE_FUNCTION.unwrap() })(val); + TEST_CALLBACK_HANDLE_FUNCTION.with_borrow(|f| { + f.expect("`TEST_CALLBACK_HANDLE_FUNCTION` must be initialized")( + val, + ); + }); }) .into_dart() } diff --git a/src/platform/dart/utils/dart_future.rs b/src/platform/dart/utils/dart_future.rs index 57abcca3..eafdc2eb 100644 --- a/src/platform/dart/utils/dart_future.rs +++ b/src/platform/dart/utils/dart_future.rs @@ -160,7 +160,7 @@ impl DartFuture { /// transferred to Dart side via `flutter_rust_bridge` bindings. #[must_use] pub fn into_dart_opaque(self) -> DartOpaque { - DartOpaque::new(self.0.cast(), unsafe { DART_HANDLER_PORT.unwrap() }) + DartOpaque::new(self.0.cast(), DART_HANDLER_PORT.get().unwrap()) } } @@ -208,6 +208,8 @@ where pub mod tests { #![expect(clippy::missing_safety_doc, reason = "for testing only")] + use std::cell::RefCell; + use dart_sys::Dart_Handle; use crate::{ @@ -250,18 +252,20 @@ pub mod tests { type TestFutureHandleFunction = extern "C" fn(Dart_Handle); - static mut TEST_FUTURE_HANDLE_FUNCTION: Option = - None; + thread_local! { + static TEST_FUTURE_HANDLE_FUNCTION: RefCell< + Option + > = RefCell::default(); + } #[no_mangle] pub unsafe extern "C" fn register__test__future_from_dart_handle_fn( f: TestFutureHandleFunction, ) { - unsafe { - TEST_FUTURE_HANDLE_FUNCTION = Some(f); - } + TEST_FUTURE_HANDLE_FUNCTION.set(Some(f)); } + #[expect(clippy::expect_used, reason = "intended behavior")] #[no_mangle] pub unsafe extern "C" fn test__future_from_dart__handle( future: Dart_Handle, @@ -272,7 +276,11 @@ pub mod tests { unsafe { FutureFromDart::execute::(future.get()) } .await .unwrap(); - (unsafe { TEST_FUTURE_HANDLE_FUNCTION.unwrap() })(val.get()); + TEST_FUTURE_HANDLE_FUNCTION.with_borrow(|f| { + f.expect("`TEST_FUTURE_HANDLE_FUNCTION` must be initialized")( + val.get(), + ); + }); Ok(()) } .into_dart_future() diff --git a/src/platform/dart/utils/string.rs b/src/platform/dart/utils/string.rs index c26a206c..19bfc707 100644 --- a/src/platform/dart/utils/string.rs +++ b/src/platform/dart/utils/string.rs @@ -1,6 +1,7 @@ //! Helper functionality for passing [`String`]s through FFI boundaries. use std::{ + cell::RefCell, ffi::{CStr, CString}, os::raw::c_char, ptr, @@ -11,10 +12,15 @@ use crate::api::propagate_panic; /// Pointer to an extern function that frees the provided Dart native string. type FreeDartNativeStringFunction = extern "C" fn(ptr::NonNull); -/// Stores a pointer to the [`FreeDartNativeStringFunction`] extern function. -/// -/// Must be initialized by Dart during FFI initialization phase. -static mut FREE_DART_NATIVE_STRING: Option = None; +thread_local! { + /// Stores a pointer to the [`FreeDartNativeStringFunction`] extern + /// function. + /// + /// Must be initialized by Dart during FFI initialization phase. + static FREE_DART_NATIVE_STRING: RefCell< + Option + > = RefCell::default(); +} /// Constructs a Rust [`String`] from the provided raw C string. /// @@ -88,20 +94,18 @@ pub unsafe extern "C" fn String_free(s: ptr::NonNull) { pub unsafe extern "C" fn register_free_dart_native_string( f: FreeDartNativeStringFunction, ) { - unsafe { - FREE_DART_NATIVE_STRING = Some(f); - } + FREE_DART_NATIVE_STRING.set(Some(f)); } /// Calls Dart to release memory allocated for the provided native string. /// -/// Should be used when Dart cannot release memory in place, e.g when Rust calls -/// a Dart function returning a native string. +/// Should be used when Dart cannot release memory in place, e.g. when Rust +/// calls a Dart function returning a native string. /// /// # Safety /// /// `FREE_DART_NATIVE_STRING` function must be registered and the provided /// pointer must be a valid native string. pub unsafe fn free_dart_native_string(s: ptr::NonNull) { - (unsafe { FREE_DART_NATIVE_STRING.unwrap() })(s); + FREE_DART_NATIVE_STRING.with_borrow(|f| f.unwrap()(s)); }