fix: manually drop room if it's last in Arc

This commit is contained in:
Artem Pylypchuk
2026-05-07 10:44:28 +03:00
committed by Damir Jelić
parent 7c13b60e28
commit c895e92c26
5 changed files with 101 additions and 4 deletions

View File

@@ -133,8 +133,10 @@ once_cell = "1.21.4"
jvm-getter = "0.1.0"
[dev-dependencies]
matrix-sdk = { workspace = true, features = ["testing"] }
similar-asserts.workspace = true
tempfile.workspace = true
tokio = { workspace = true, features = ["rt-multi-thread", "macros", "time"] }
[build-dependencies]
uniffi = { workspace = true, features = ["build"] }

View File

@@ -3367,4 +3367,51 @@ mod tests {
assert_eq!(token.matrix_server_name, "example.com");
assert_eq!(token.expires_in_seconds, 3_600);
}
/// Dropping an FFI [`Client`] on a non-tokio thread must not panic.
///
/// Regression test: `Client.inner` is wrapped in [`AsyncRuntimeDropped`]
/// precisely because dropping a sqlite-backed [`MatrixClient`] outside a
/// tokio runtime context causes `SyncWrapper<rusqlite::Connection>::drop`
/// to call `tokio::task::spawn_blocking`, which panics and triggers
/// SIGABRT. This test guards against accidentally removing that wrapper.
#[cfg(not(target_family = "wasm"))]
#[tokio::test]
async fn client_drop_on_non_tokio_thread_does_not_panic() {
use std::time::Duration;
use matrix_sdk::{Client, config::RequestConfig};
use matrix_sdk_common::cross_process_lock::CrossProcessLockConfig;
use tempfile::tempdir;
let dir = tempdir().unwrap();
// SingleProcess lock avoids the session-delegate requirement in
// `Client::new`, keeping the test self-contained.
let sdk_client = Client::builder()
.homeserver_url("https://example.com")
.request_config(RequestConfig::new().disable_retry())
.sqlite_store(dir.path(), None)
.cross_process_store_config(CrossProcessLockConfig::SingleProcess)
.build()
.await
.unwrap();
// Allow background init tasks to complete; after this the only strong
// reference to `ClientInner` is the local `sdk_client` variable.
tokio::time::sleep(Duration::from_secs(1)).await;
let ffi_client = super::Client::new(sdk_client.clone(), None, None).await.unwrap();
// Dropping `sdk_client` leaves `ffi_client` as the sole Arc holder of
// `ClientInner` — mirroring what happens when the last JS reference to
// a Client is garbage-collected on the Hermes JS thread.
drop(sdk_client);
// Simulate Hermes GC on the JS thread (a non-tokio thread).
// Without `AsyncRuntimeDropped` wrapping `Client.inner` this SIGABRT.
std::thread::spawn(move || drop(ffi_client))
.join()
.expect("Client::drop panicked on a non-tokio thread");
}
}

View File

@@ -98,13 +98,13 @@ impl From<RoomState> for Membership {
#[derive(uniffi::Object)]
pub struct Room {
pub(super) inner: SdkRoom,
pub(super) inner: AsyncRuntimeDropped<SdkRoom>,
utd_hook_manager: Option<Arc<UtdHookManager>>,
}
impl Room {
pub(crate) fn new(inner: SdkRoom, utd_hook_manager: Option<Arc<UtdHookManager>>) -> Self {
Room { inner, utd_hook_manager }
Room { inner: AsyncRuntimeDropped::new(inner), utd_hook_manager }
}
}
@@ -1981,3 +1981,51 @@ impl TryFrom<SdkRoomSendQueueUpdate> for RoomSendQueueUpdate {
})
}
}
#[cfg(all(test, not(target_family = "wasm")))]
mod tests {
use std::time::Duration;
use matrix_sdk::{ruma::room_id, test_utils::mocks::MatrixMockServer};
use tempfile::tempdir;
use super::*;
/// Dropping an FFI [`Room`] on a non-tokio thread must not panic.
///
/// Regression test: when `Room.inner` was `SdkRoom` (not wrapped in
/// [`AsyncRuntimeDropped`]), garbage-collection of an orphaned `Room` on
/// the Hermes JS thread caused `SyncWrapper<rusqlite::Connection>::drop`
/// to call `tokio::task::spawn_blocking` from a thread with no tokio
/// runtime context. Rust turns a panic inside `Drop` into SIGABRT.
///
/// The fix wraps `inner` in [`AsyncRuntimeDropped`], which enters the
/// global tokio runtime context before running `SdkRoom`'s destructor.
#[tokio::test]
async fn room_drop_on_non_tokio_thread_does_not_panic() {
let server = MatrixMockServer::new().await;
let dir = tempdir().unwrap();
let client =
server.client_builder().on_builder(|b| b.sqlite_store(dir.path(), None)).build().await;
// Allow background init tasks to complete; after this the only strong
// reference to `ClientInner` is the local `client` variable.
tokio::time::sleep(Duration::from_secs(1)).await;
let room_id = room_id!("!test:example.com");
let sdk_room = server.sync_joined_room(&client, room_id).await;
let ffi_room = Room::new(sdk_room, None);
// Dropping `client` makes `ffi_room` the sole Arc holder of
// `ClientInner`, mirroring the situation where the FFI client has
// already been released and GC finalises the last room JS object.
drop(client);
// Simulate Hermes GC on the JS thread (a non-tokio thread).
// Without the `AsyncRuntimeDropped` wrapper this causes a SIGABRT.
std::thread::spawn(move || drop(ffi_room))
.join()
.expect("Room::drop panicked on a non-tokio thread");
}
}

View File

@@ -54,7 +54,7 @@ impl Room {
/// a time.
pub fn search_messages(&self, query: String, num_results_per_batch: u32) -> RoomSearchIterator {
RoomSearchIterator {
sdk_room: self.inner.clone(),
sdk_room: (*self.inner).clone(),
inner: Mutex::new(self.inner.search_messages(query, num_results_per_batch as usize)),
}
}

View File

@@ -55,7 +55,7 @@ impl WidgetDriver {
};
let capabilities_provider = CapabilitiesProviderWrap(capabilities_provider.into());
if let Err(()) = driver.run(room.inner.clone(), capabilities_provider).await {
if let Err(()) = driver.run((*room.inner).clone(), capabilities_provider).await {
// TODO
}
}