fix(ffi): manually drop room if it's last in Arc#6545
Open
articice wants to merge 1 commit intomatrix-org:mainfrom
Open
fix(ffi): manually drop room if it's last in Arc#6545articice wants to merge 1 commit intomatrix-org:mainfrom
articice wants to merge 1 commit intomatrix-org:mainfrom
Conversation
352b7d9 to
66c5f80
Compare
Merging this PR will degrade performance by 49.4%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | Restore session [memory store] |
113.9 ms | 225.1 ms | -49.4% |
Comparing articice:manually-drop-room (b78d14a) with main (777ce05)
66c5f80 to
b78d14a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6545 +/- ##
==========================================
- Coverage 89.93% 89.90% -0.04%
==========================================
Files 381 381
Lines 106780 106780
Branches 106780 106780
==========================================
- Hits 96036 96002 -34
- Misses 7096 7116 +20
- Partials 3648 3662 +14 ☔ View full report in Codecov by Sentry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The FFI Client.inner is already wrapped in AsyncRuntimeDropped because dropping a sqlite-backed MatrixClient outside a tokio runtime causes
SyncWrapper<rusqlite::Connection>::dropto calltokio::task::spawn_blocking. Without runtime context that panics, and a panic inside Drop aborts the process (SIGABRT).Room.inner: SdkRoomwas not wrapped. AnSdkRoomholds anArc<ClientInner>. When the FFI Client had already been released and React Native's Hermes GC finalized the last JS reference to a Room on the JS thread (non-tokio),Room::drop→SdkRoom::drop→ lastArc<ClientInner>::drop→sqlitedrop →spawn_blockingpanic → SIGABRT.This completes the pattern from #4168, with #2815 / #2821 being reference fixes for deadpool/sqlite drop-context root cause.
TL;DR
Element X (iOS/Android) and other Tauri-style consumers don't hit this because they keep a single long-lived Client for the app/session lifetime, so Room drops never release the last Arc. React Native + Hermes hits it because the JS Client can be released while old Room JS references await GC on a non-tokio thread; when Hermes finalizes them, the drop chain reaches sqlite::Connection::drop → spawn_blocking outside any runtime → SIGABRT.
The wrapper fixes that one corner case without affecting any other consumer.
TimelineFFI type is the next strongest candidate, will follow-up.History of the Drop-on-non-tokio-thread problem
2023 — original problem and first hack
2024 — pattern proliferation, then helper
RoomListItem::preview_room#4152 chore(room_preview): add RoomListItem::preview_room — Oct 25 2024 (commit 40f4fc1)AsyncRuntimeDroppedhelper #4168 chore(ffi): introduce AsyncRuntimeDropped helper — Benjamin Bouvier, Oct 25 2024 (commit c08194a)2025 — wasm extension
2026 — this PR
AsyncRuntimeDroppedhelper #4168 left half-finished.Why upstream didn't catch it
thread. SdkRoom → Arc last-strong-drop → SyncWrapperrusqlite::Connection::drop → tokio::task::spawn_blocking from a non-tokio thread → panic in Drop → SIGABRT.
CHANGELOG.mdfiles.Signed-off-by: