send queue: allow sending reactions to local echoes #3749

bnjbvr · 2024-07-22T16:49:19Z

codecov · 2024-08-13T07:40:49Z

Codecov Report

Attention: Patch coverage is 85.15284% with 34 lines in your changes missing coverage. Please review.

Project coverage is 84.13%. Comparing base (ece3305) to head (b94a6e6).
Report is 11 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/inner/mod.rs	75.51%	24 Missing ⚠️
crates/matrix-sdk/src/send_queue.rs	91.50%	9 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/util.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3749      +/-   ##
==========================================
+ Coverage   84.09%   84.13%   +0.04%     
==========================================
  Files         266      266              
  Lines       27861    27996     +135     
==========================================
+ Hits        23429    23554     +125     
- Misses       4432     4442      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-08-27T15:53:02Z

So, some jobs have been failing here with an error like this:

error[E0275]: overflow evaluating the requirement `matrix_sdk::ruma::ruma_events::OriginalSyncMessageLikeEvent<matrix_sdk::ruma::ruma_events::room::redaction::RoomRedactionEventContent>: std::marker::Send`
    |
    = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`multiverse`)

We're making a library, so it's a big no-no to add the requirement that every user adds the recursion_limit to their crate, just because they're using ours. So I digged a bit, and found another solution.

The issue is that when trying to evaluate if a type is Send / Sync, rustc will do recursive calls for deeply nested types, until it stops doing so because it thinks it may be blowing up the stack.

A workaround that's been explained to me by a rustc friend: it's possible to "break up" the call tree by adding an unsafe impl Send (resp. Sync) for one of the types in the tree. I've done that locally for SyncTimelineEvent, which always appears in the error traces.

Now, it happens that SyncTimelineEvent is Send and Sync right now, but that might not be true again in the future. So I'd really like the compiler to tell me whenever it becomes not Send or Sync. As a result, I've introduced a new Cargo feature for the matrix-sdk-common crate (that contains the SyncTimelineEvent), called test-sync-send:

when this Cargo feature is set, the unsafe impls are not defined, and a test will statically assert that this type implements the Send and Sync bounds. Basically, back to square one: the compiler does the work of proving this specific type (and not users of it) is Send and Sync. One of the CI jobs will run with this feature.
when the Cargo feature isn't set, this test is disabled, and the unsafe impls are active, so the compiler can short-cut proving that this type is Send and Sync, and we don't hit the recursion limit.

This is a bit hacky, and I hope the next solver will do better; in the meanwhile, is it an acceptable tradeoff?

[edit] doesn't work for the doc build, which hits the same issue involving only Ruma types, it seems 👀

[edit 2] works when put on matrix-sdk-base::Room, oh well 🤷

…unds in rustc See #3749 (comment) for the gory details.

bmarty · 2024-08-28T10:15:45Z

Hello @bnjbvr , testing the branch on EXA.

Note: I use the eventId for the uniqueId parameter here, this is maybe not correct, let me know. (EDIT 2: this is probably the root cause of the issue below, give me a minute)

in online mode:

send a message. the message is sent.
switch off the network (airplane mode for instance)
send a reaction. Nothing happen on the timeline (no local echo) and I get this log:

elementx: org.matrix.rustcomponents.sdk.ClientException$Generic: msg=Failed toggling reaction
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.read(matrix_sdk_ffi.kt:25559)
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.read(matrix_sdk_ffi.kt:25554)
at org.matrix.rustcomponents.sdk.FfiConverter$DefaultImpls.liftFromRustBuffer(matrix_sdk_ffi.kt:225)
at org.matrix.rustcomponents.sdk.FfiConverterRustBuffer$DefaultImpls.liftFromRustBuffer(matrix_sdk_ffi.kt:237)
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.liftFromRustBuffer(matrix_sdk_ffi.kt:25554)
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.liftFromRustBuffer(matrix_sdk_ffi.kt:25554)
at org.matrix.rustcomponents.sdk.FfiConverterRustBuffer$DefaultImpls.lift(matrix_sdk_ffi.kt:238)
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.lift(matrix_sdk_ffi.kt:25554)
at org.matrix.rustcomponents.sdk.FfiConverterTypeClientError.lift(matrix_sdk_ffi.kt:25554)
at org.matrix.rustcomponents.sdk.ClientException$ErrorHandler.lift(matrix_sdk_ffi.kt:25548)
at org.matrix.rustcomponents.sdk.ClientException$ErrorHandler.lift(matrix_sdk_ffi.kt:25547)
at org.matrix.rustcomponents.sdk.Matrix_sdk_ffiKt.uniffiCheckCallStatus(matrix_sdk_ffi.kt:301)
at org.matrix.rustcomponents.sdk.Matrix_sdk_ffiKt.access$uniffiCheckCallStatus(matrix_sdk_ffi.kt:1)
at org.matrix.rustcomponents.sdk.Matrix_sdk_ffiKt.uniffiRustCallAsync(matrix_sdk_ffi.kt:36163)
at org.matrix.rustcomponents.sdk.Timeline.toggleReaction$suspendImpl(matrix_sdk_ffi.kt:19773)
at org.matrix.rustcomponents.sdk.Timeline.toggleReaction(Unknown Source:0)

I cannot test adding reaction on not sent message, I need to do some change on the application to be able to do that, but I will let you know what the result of this test.

bnjbvr · 2024-08-28T10:28:58Z

Note: I use the eventId for the uniqueId parameter here, this is maybe not correct, let me know. (EDIT 2: this is probably the root cause of the issue below, give me a minute)

@bmarty Yeah, you need to pass the "internal_id()" or "unique_id()" for the item; this was a way to avoid having to deal with passing an EventTimelineItem. I'll tweak the documentation later (after review) to make this clearer.

bmarty · 2024-08-28T10:41:02Z

OK, thanks, passing the unique_id is solving the issue, and is working well as well when reacted to a non sent event.

I have found a scenario which is failing though:

online mode
send a message
add a reaction, and wait for everything to be synced.
switch to offline
redact the reaction
react again with the same reaction
switch to online

Nothing should happen, but at the end the reaction is redacted.

See the video below:

Hywan

Lot of code. Not sure I'm having all the details but it looks good! Well done.

Now, let's see how many persons will react to their own local event…

Hywan · 2024-08-28T12:36:53Z

crates/matrix-sdk/src/send_queue.rs

+#[derive(Clone, Debug)]
+pub enum LocalEchoContent {
+    /// The local echo contains an actual event ready to display.
+    Event {


Do you imagine adding Media in a close future?

Yes, that's the idea!

Hywan · 2024-08-28T12:55:19Z

crates/matrix-sdk/src/send_queue.rs

@@ -1019,6 +1019,9 @@ impl QueueStorage {
            .map_err(RoomSendQueueStorageError::StorageError)?;

        let num_initial_dependent_events = dependent_events.len();
+        if num_initial_dependent_events == 0 {
+            return Ok(());


Why is it useful to return early? To avoid calling canonicalize_dependent_events? Is it necessary to add a comment?

It avoided a few spurious log lines below; will comment briefly there.

Hywan · 2024-08-28T12:56:51Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

-            let Some(remote_event_item) = event_item.as_remote() else {
-                error!("received reaction to a local echo");
-                return;
-            };


Hywan · 2024-08-28T12:59:17Z

crates/matrix-sdk-base/src/rooms/normal.rs

+// See https://github.com/matrix-org/matrix-rust-sdk/pull/3749#issuecomment-2312939823.
+#[cfg(not(feature = "test-send-sync"))]
+unsafe impl Send for Room {}
+
+// See https://github.com/matrix-org/matrix-rust-sdk/pull/3749#issuecomment-2312939823.
+#[cfg(not(feature = "test-send-sync"))]
+unsafe impl Sync for Room {}
+
+#[cfg(feature = "test-send-sync")]
+#[test]
+// See https://github.com/matrix-org/matrix-rust-sdk/pull/3749#issuecomment-2312939823.
+fn test_send_sync_for_room() {
+    fn assert_send_sync<T: Send + Sync>() {}
+
+    assert_send_sync::<Room>();
+}


…ds in rustc See #3749 (comment) for the gory details.

bnjbvr · 2024-08-28T14:49:44Z

@bmarty opened a followup for the issue you've found, since it's unfortunately not trivial to handle. Thanks though!

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from 106b552 to 69b9379 Compare August 13, 2024 07:24

bnjbvr changed the title ~~send queue: use the send queue for reactions~~ send queue: allow sending reactions to local echoes Aug 20, 2024

bnjbvr mentioned this pull request Aug 20, 2024

timeline: only use the send queue mechanism to send reactions #3865

Merged

bnjbvr force-pushed the bnjbvr/send-q-reactions branch 3 times, most recently from 6611815 to 4835265 Compare August 26, 2024 16:23

bnjbvr added a commit that referenced this pull request Aug 27, 2024

sdk-common: hack to avoid over-recursion when evaluating Send/Sync bo…

3fc561d

…unds in rustc See #3749 (comment) for the gory details.

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from 4835265 to 3fc561d Compare August 27, 2024 16:06

bnjbvr added a commit that referenced this pull request Aug 27, 2024

sdk-common: hack to avoid over-recursion when evaluating Send/Sync bo…

c9e9580

…unds in rustc See #3749 (comment) for the gory details.

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from 3fc561d to c9e9580 Compare August 27, 2024 16:08

bnjbvr added a commit that referenced this pull request Aug 27, 2024

sdk-common: hack to avoid over-recursion when evaluating Send/Sync bo…

a83f6b6

…unds in rustc See #3749 (comment) for the gory details.

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from c9e9580 to a83f6b6 Compare August 27, 2024 16:48

bnjbvr marked this pull request as ready for review August 27, 2024 16:49

bnjbvr requested a review from a team as a code owner August 27, 2024 16:49

bnjbvr requested review from Hywan and removed request for a team August 27, 2024 16:49

bnjbvr and others added 3 commits August 28, 2024 12:04

send queue: put a LocalEcho's event content into a new enum

4fcadf8

send queue: handle reactions in the send queue

fa14e5f

send queue: add integration test for sending and aborting reactions

40e09bb

bnjbvr added a commit that referenced this pull request Aug 28, 2024

sdk-common: hack to avoid over-recursion when evaluating Send/Sync bo…

a802940

…unds in rustc See #3749 (comment) for the gory details.

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from a83f6b6 to a802940 Compare August 28, 2024 10:04

Hywan approved these changes Aug 28, 2024

View reviewed changes

bmarty mentioned this pull request Aug 28, 2024

Allow reaction to non sent Event (local echo for reaction) element-hq/element-x-android#3348

Closed

10 tasks

bnjbvr mentioned this pull request Aug 28, 2024

local reactions followup: redacting then adding reaction fails in subtle ways #3901

Closed

send queue: early return if there are no dependent events to handle

36df618

bnjbvr added 3 commits August 28, 2024 16:13

timeline: allow reactions in local|remote event timeline items

1b9f190

timeline: add support for local reactions to local echoes

b7b665d

sdk-base: hack to avoid over-recursion when evaluating Send/Sync boun…

b94a6e6

…ds in rustc See #3749 (comment) for the gory details.

bnjbvr force-pushed the bnjbvr/send-q-reactions branch from a802940 to b94a6e6 Compare August 28, 2024 14:14

bnjbvr merged commit 47444cc into main Aug 28, 2024
39 checks passed

bnjbvr deleted the bnjbvr/send-q-reactions branch August 28, 2024 14:49

bnjbvr mentioned this pull request Oct 17, 2024

Allow adding a reaction to a local echo of a message #2127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

send queue: allow sending reactions to local echoes #3749

send queue: allow sending reactions to local echoes #3749

bnjbvr commented Jul 22, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

bnjbvr commented Aug 27, 2024 •

edited

Loading

bmarty commented Aug 28, 2024 •

edited

Loading

bnjbvr commented Aug 28, 2024

bmarty commented Aug 28, 2024

Hywan left a comment

Hywan Aug 28, 2024

bnjbvr Aug 28, 2024

Hywan Aug 28, 2024

bnjbvr Aug 28, 2024

Hywan Aug 28, 2024

Hywan Aug 28, 2024

bnjbvr commented Aug 28, 2024

send queue: allow sending reactions to local echoes #3749

send queue: allow sending reactions to local echoes #3749

Conversation

bnjbvr commented Jul 22, 2024 • edited Loading

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

bnjbvr commented Aug 27, 2024 • edited Loading

bmarty commented Aug 28, 2024 • edited Loading

bnjbvr commented Aug 28, 2024

bmarty commented Aug 28, 2024

Hywan left a comment

Choose a reason for hiding this comment

Hywan Aug 28, 2024

Choose a reason for hiding this comment

bnjbvr Aug 28, 2024

Choose a reason for hiding this comment

Hywan Aug 28, 2024

Choose a reason for hiding this comment

bnjbvr Aug 28, 2024

Choose a reason for hiding this comment

Hywan Aug 28, 2024

Choose a reason for hiding this comment

Hywan Aug 28, 2024

Choose a reason for hiding this comment

bnjbvr commented Aug 28, 2024

bnjbvr commented Jul 22, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

bnjbvr commented Aug 27, 2024 •

edited

Loading

bmarty commented Aug 28, 2024 •

edited

Loading