Skip to content

feat(sdk): Introduce RoomEventCacheGenericUpdate, one channel to get update of all rooms#5247

Merged
Hywan merged 4 commits intomatrix-org:mainfrom
Hywan:feat-sdk-event-cache-subscribe-to-sync
Jun 24, 2025
Merged

feat(sdk): Introduce RoomEventCacheGenericUpdate, one channel to get update of all rooms#5247
Hywan merged 4 commits intomatrix-org:mainfrom
Hywan:feat-sdk-event-cache-subscribe-to-sync

Conversation

@Hywan
Copy link
Member

@Hywan Hywan commented Jun 18, 2025

RoomEventCache::subscribe is nice to subscribe to every update
happening inside a room in the event cache. However, the returned
RoomEventCacheSubscriber has side-effects when dropped (see
auto-shrink to save memory space). In some situation, this is pretty
annoying. for example, if one wants to listen to multiple room updates,
all the room event cache subscribers must be kept in memory, thus
breaking the side-effects. This isn't always the desired output. In
addition, listening to multiple channels/subscribers at the same is
quite complex, as it implies non-trivial async runtime efforts or
complex future types.

To solve this problem, this patch introduces a new
EventCache::subscribe_to_room_generic_updates method, which returns a
single Receiver<RoomEventCacheGenericUpdate>.

First off, it hides the details of RoomEventCacheUpdate (returned by
RoomEventCacheSubscriber), which might be desired, but particularly
lighter because events aren't part of the payload.

Second, one no longer needs to subscribe to all rooms. Only one channel
can be listened to get updates for all rooms. It reduces the complexity
on the caller side, plus Receiver<RoomEventCacheGenericUpdate> doesn't
have any side-effect.

This patch tests this feature in 4 situations:

  1. when a room is created/loaded empty,
  2. when a room is loaded and is not empty because data exists in the storage,
  3. when a room receives data from the sync,
  4. when a room receives data from the pagination.

Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Jun 18, 2025
@Hywan Hywan marked this pull request as ready for review June 18, 2025 15:37
@Hywan Hywan requested a review from a team as a code owner June 18, 2025 15:37
@Hywan Hywan requested review from andybalaam and removed request for a team June 18, 2025 15:37
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really not a big fan of random renamings, as I find it confusing, as a very active developer in the event cache, to have concepts I'm working on being "disappearing" under my feet. We could probably discuss this practice in our face-to-face time, because this is really frustrating me, but maybe it doesn't have to.

Other than that, the patch makes sense to me overall. Not a huge fan of the super long name, but maybe we can find a shorter one. How do you plan to use this, out of curiosity?

@bnjbvr bnjbvr removed the request for review from andybalaam June 19, 2025 08:56
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with a couple of questions. Thanks!

Hywan added a commit to Hywan/matrix-rust-sdk that referenced this pull request Jun 23, 2025
@Hywan Hywan force-pushed the feat-sdk-event-cache-subscribe-to-sync branch from bc568fc to b065439 Compare June 23, 2025 13:20
@codecov
Copy link

codecov bot commented Jun 23, 2025

Codecov Report

Attention: Patch coverage is 96.75090% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (4025c11) to head (ecb0698).
Report is 10 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/event_cache/room/mod.rs 92.00% 0 Missing and 6 partials ⚠️
crates/matrix-sdk/src/event_cache/mod.rs 98.45% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5247      +/-   ##
==========================================
+ Coverage   90.18%   90.22%   +0.03%     
==========================================
  Files         334      334              
  Lines      104824   105100     +276     
  Branches   104824   105100     +276     
==========================================
+ Hits        94540    94828     +288     
+ Misses       6232     6212      -20     
- Partials     4052     4060       +8     

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

@Hywan Hywan force-pushed the feat-sdk-event-cache-subscribe-to-sync branch 2 times, most recently from 21f3a9a to 6b98245 Compare June 23, 2025 15:21
@Hywan Hywan changed the title feat(sdk): Introduce GenericRoomEventCacheUpdate, one channel to get update of all rooms feat(sdk): Introduce RoomEventCacheGenericUpdate, one channel to get update of all rooms Jun 23, 2025
Hywan added 2 commits June 23, 2025 17:26
This patch fixes a comment about `RoomEventCacheState::handle_sync`
returned values.
…t update of all rooms.

`RoomEventCache::subscribe` is nice to subscribe to every update
happening inside a room in the event cache. However, the returned
`RoomEventCacheSubscriber` has side-effects when dropped (see
auto-shrink to save memory space). In some situation, this is pretty
annoying. for example, if one wants to listen to multiple room updates,
all the room event cache subscribers must be kept in memory, thus
breaking the side-effects. This isn't always the desired output. In
addition, listening to multiple channels/subscribers at the same is
quite complex, as it implies non-trivial async runtime efforts or
complex future types.

To solve this problem, this patch introduces a new
`EventCache::subscribe_to_room_generic_updates` method, which returns a
single `Receiver<RoomEventCacheGenericUpdate>`.

First off, it hides the details of `RoomEventCacheUpdate` (returned by
`RoomEventCacheSubscriber`), which might be desired, but particularly
lighter because events aren't part of the payload.

Second, one no longer needs to subscribe to all rooms. Only one channel
can be listened to get updates for all rooms. It reduces the complexity
on the caller side, plus `Receiver<RoomEventCacheGenericUpdate>` doesn't
have any side-effect.

This patch tests this feature in 4 situations:

1. when a room is created/loaded empty,
2. when a room is loaded and is not empty because data exists in the storage,
3. when a room receives data from the sync,
4. when a room receives data from the pagination.
@Hywan Hywan force-pushed the feat-sdk-event-cache-subscribe-to-sync branch from 6b98245 to c30aa72 Compare June 23, 2025 15:27
@Hywan Hywan requested a review from bnjbvr June 23, 2025 15:31
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

// Create 1 room, with 4 chunks in the event cache storage.
let user = user_id!("@mnt_io:matrix.org");
let client = logged_in_client(None).await;
let room_id = room_id!("!raclette:patate.ch");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yummy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫕

@Hywan Hywan merged commit e0ab16f into matrix-org:main Jun 24, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants