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

Fix endless loading timeline due to conflicting chunks #5554

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

SpiritCroc
Copy link
Contributor

I ran into following scenario recently: at some point, scrolling down to more recent messages wasn't working anymore, it would endlessly show the loading animation. This was reproducible, so I started debugging. It turned out:

  • The TimelineChunk I was viewing had chunkEntity.nextChunk == null.
  • When trying to download a new chunk, TokenChunkEventPersistor aborted, as the next chunk was already in the database.

It turned out that I had multiple chunks in my DB, so only one had the correct reference to nextChunk, while the other one, which I was viewing, was stuck at loading forever.

This commit works around this by "repairing" links in this case. I assume there is a better way/place to fix it, but this works for me for now. I figured it might still be interesting for you, maybe you have some suggestions how to better fix it.

Signed-off-by: Tobias Büttner [email protected]

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 11

Checklist

The previous fix only works around the issue when it is detected. This
may require re-entering the room once when it gets stuck. If we ensure
proper linking from the beginning, hopefully we don't run into any
issues at all.
@ouchadam ouchadam added the Z-Community-PR Issue is solved by a community member's PR label Mar 18, 2022
@ganfra
Copy link
Member

ganfra commented Apr 8, 2022

Thats weird you got multiple same chunk in the db with only one valid...

@SpiritCroc
Copy link
Contributor Author

SpiritCroc commented Apr 8, 2022

This seems to happen more often though, we got a few user reports in the past of similar endless loading issues. With the first commit, such issues still happened, but got fixed after re-opening the room. Since the second commit, I haven't observed any new reports that sound like this issue.
(Note that these commits, such as most of my bugfix PRs, are already in use on the Beeper and SchildiChat Android apps, so there are more users testing these in addition to myself).

@ganfra
Copy link
Member

ganfra commented Apr 8, 2022

Thanks, I'll check a bit!
Are you working at Beeper now? :D

@SpiritCroc
Copy link
Contributor Author

SpiritCroc commented Apr 8, 2022

Yes, (part-time) since around last year April, Beeper-Android was technically a fork of SchildiChat initially :)

@@ -80,7 +81,26 @@ internal class TokenChunkEventPersistor @Inject constructor(

val existingChunk = ChunkEntity.find(realm, roomId, prevToken = prevToken, nextToken = nextToken)
if (existingChunk != null) {
Timber.v("This chunk is already in the db, returns")
Timber.v("This chunk is already in the db, checking if this might be caused by broken links")
Copy link
Member

Choose a reason for hiding this comment

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

Can you just create a method to keep this a bit smaller

Copy link
Member

Choose a reason for hiding this comment

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

By the way, is this still necessary with your second commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can probably do that this weekend or next week.
It's probably only needed if we want to retrospectively fix existing cases of the issue without requiring users to do a fresh initial sync to fix it. Commit two should avoid this scenario, if I haven't missed any case.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this should, even if there is probably another issue underneath as I don't think we should have multiple same chunks in the db :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still have some issues with timeline chunk loops, like in this issue: #5092. With some added debugging, I've seen loops starting with 4 chunks, and up to 81 chunks forming a loop (timeline chunks linking forwards and backwards indirectly to the same chunk). Seems more likely to happen while viewing a room in which spam is going on. I wonder if those loops are caused by a related bug to this one.

Copy link
Member

Choose a reason for hiding this comment

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

it's kind of hard to replicate, but there is something wrong definitively.
Nevermind, let's merge this to mitigate. Thanks!

@ganfra ganfra merged commit f7e8c01 into element-hq:develop Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants