-
Notifications
You must be signed in to change notification settings - Fork 362
fix(sdk): Fix sticky local latest event #5968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(sdk): Fix sticky local latest event #5968
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5968 +/- ##
========================================
Coverage 88.51% 88.51%
========================================
Files 363 363
Lines 103490 103590 +100
Branches 103490 103590 +100
========================================
+ Hits 91599 91696 +97
Misses 7532 7532
- Partials 4359 4362 +3 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5968 will not alter performanceComparing Summary
|
11e07d3 to
5256dba
Compare
The problem we are trying to solve is the following: - a local event is being sent, - the `LatestEventValue` is `LocalIsSending`, - the local event is finally sent, - the `LatestEventValue` is still `LocalIsSending` purposely, with the hope that an update from the Event Cache will replace it. But sometimes, this update from the Event Cache comes **before** the update from the Send Queue. Why is it problem? Because updates from the Event Cache are ignored until the buffer of local `LatestEventValue`s aren't empty, which means that if an update from the Event Cache is received before `RoomSendQueueUpdate::SentEvent`, it is ignored, and the `LatestEventValue` stays in the `LocalIsSending` state. That's annoying. The idea is to introduce a new state: `LocalHasBeenSent` which mimics `Remote`, but for a local event. It clarifies the state of a sent event, without relying on the Event Cache.
5256dba to
1c7d1b6
Compare
poljar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
…enSent`. This patch changes the semantics of the Room List `latest_event` sorter by changing “is local” to “is remote like”, to include the new `LatestEventValue::LocalHasBeenSent` variant.
2d6de7b to
db6684a
Compare
Based on top of #5947. Must be merged before this one.
The problem we are trying to solve is the following:
LatestEventValueisLocalIsSending,LatestEventValueis stillLocalIsSendingpurposely, with the hope that an update from the Event Cache will replace it, thus becoming aRemote.But sometimes, this update from the Event Cache comes before the update from the Send Queue because the whole mechanism is async with no ordering guarantee. Why is it problem? Because updates from the Event Cache are ignored until the buffer of local
LatestEventValues aren't empty (so that local latest event cannot be overwritten and has the priority). It means that if the update from the Event Cache is received beforeRoomSendQueueUpdate::SentEvent, it is ignored, and theLatestEventValuestays in theLocalIsSendingstate. That's annoying.The idea is to introduce a new state:
LocalHasBeenSentwhich mimicsRemote, but for a local event. It clarifies the state of a sent event, without relying on the Event Cache.The patch is twofold:
LatestEventValue::LocalHasBeenSentvariant,latest_eventRoom List sorter to handle this new case.