Skip to content
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

[timeline/event-cache] a timeline clear should cause a cached events read from the event cache #3311

Closed
bnjbvr opened this issue Apr 9, 2024 · 1 comment · Fixed by #3345
Assignees

Comments

@bnjbvr
Copy link
Member

bnjbvr commented Apr 9, 2024

Rough notes from an internal chat room:

FYI, there are multiple reports of the timeline "lagging behind" the room event cache updates, like https://github.com/element-hq/element-x-ios-rageshakes/issues/1674#issuecomment-2045118673

This happens because the timeline can't keep up with the rate of room event cache updates, which is super surprising? the timeline shouldn't run code for very long, in theory, so there's more to untangle there. We'd need to investigate a bit more to understand why this happens, and if there are ways to mitigate that.

One obvious way would be to add an additional task that just forwards cache updates into an unbounded queue, but I wouldn't add more concurrency to the mix just yet.

Now, I think that was confuses people is they see old events, and interpret this as missing chunks. My WIP theory is the following:

  • back-pagination is happening in the background
  • the timeline is cleared because of a lag behind the room event cache updates
  • back-pagination finishes, and old events are appended to the timeline because of that.

Now, this is the same failure mode as in the "when ignoring a user, you may see old items", with the same reason, except #2 is a timeline clear because we ignored a user.

In all cases, we expect the caller to retrigger back-pagination manually, but in this case it would be wrong, because there are already back-paginated events in the timeline.

oh this ain't gonna work, since it's event cache handling the back-pagination state now 🤦

When clearing the timeline, we should append all the events from the event cache

Part of #3058

@bnjbvr
Copy link
Member Author

bnjbvr commented Apr 19, 2024

After thinking about it, we only need to do this upon lag behind the event cache. In other cases, we might be clearing the room with a different intent, so retrieving the events from the cache would not be the right move.

bnjbvr added a commit that referenced this issue Apr 19, 2024
… from the cache

Fixes #3311.

When the timeline is lagging behind the event cache, it should not only
clear what it contains (because it may be lagging some information
coming from the event cache), it should also retrieve the events the
cache knows about, and adds them as if they were "initial" events.

This makes sure that the event cache's events and the timeline's events
are always the same, and that, in the case of a lag, there won't be any
missing chunks (caused by the event cache back-pagination being further
away in the past, than what's displayed in the timeline).

The lag behind a bit too hard to reproduce, I've not included a test
here; but I think this should be the right move.
bnjbvr added a commit that referenced this issue Apr 19, 2024
… from the cache

Fixes #3311.

When the timeline is lagging behind the event cache, it should not only
clear what it contains (because it may be lagging some information
coming from the event cache), it should also retrieve the events the
cache knows about, and adds them as if they were "initial" events.

This makes sure that the event cache's events and the timeline's events
are always the same, and that, in the case of a lag, there won't be any
missing chunks (caused by the event cache back-pagination being further
away in the past, than what's displayed in the timeline).

The lag behind a bit too hard to reproduce, I've not included a test
here; but I think this should be the right move.
bnjbvr added a commit that referenced this issue Apr 22, 2024
… from the cache

Fixes #3311.

When the timeline is lagging behind the event cache, it should not only
clear what it contains (because it may be lagging some information
coming from the event cache), it should also retrieve the events the
cache knows about, and adds them as if they were "initial" events.

This makes sure that the event cache's events and the timeline's events
are always the same, and that, in the case of a lag, there won't be any
missing chunks (caused by the event cache back-pagination being further
away in the past, than what's displayed in the timeline).

The lag behind a bit too hard to reproduce, I've not included a test
here; but I think this should be the right move.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant