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.

Sign up for GitHub

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
Merged

Fix endless loading timeline due to conflicting chunks #5554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
682f4c3
Fix endless loading timeline due to conflicting chunks
SpiritCroc Mar 16, 2022
6878a97
TokenChunkEventPersistor: always link all matching chunks
SpiritCroc Mar 18, 2022
902d2f7
TokenChunkEventPersistor: move link fixing to its own method
SpiritCroc Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5554.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix sometimes endless loading timeline
11 changes: 11 additions & 0 deletions ...ndroid/src/main/java/org/matrix/android/sdk/internal/database/query/ChunkEntityQueries.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ internal fun ChunkEntity.Companion.find(realm: Realm, roomId: String, prevToken:
return query.findFirst()
}

internal fun ChunkEntity.Companion.findAll(realm: Realm, roomId: String, prevToken: String? = null, nextToken: String? = null): RealmResults<ChunkEntity>? {
val query = where(realm, roomId)
if (prevToken != null) {
query.equalTo(ChunkEntityFields.PREV_TOKEN, prevToken)
}
if (nextToken != null) {
query.equalTo(ChunkEntityFields.NEXT_TOKEN, nextToken)
}
return query.findAll()
}

internal fun ChunkEntity.Companion.findLastForwardChunkOfRoom(realm: Realm, roomId: String): ChunkEntity? {
return where(realm, roomId)
.equalTo(ChunkEntityFields.IS_LAST_FORWARD, true)
Expand Down
42 changes: 39 additions & 3 deletions ...in/java/org/matrix/android/sdk/internal/session/room/timeline/TokenChunkEventPersistor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import org.matrix.android.sdk.internal.database.model.TimelineEventEntityFields
import org.matrix.android.sdk.internal.database.query.copyToRealmOrIgnore
import org.matrix.android.sdk.internal.database.query.create
import org.matrix.android.sdk.internal.database.query.find
import org.matrix.android.sdk.internal.database.query.findAll
import org.matrix.android.sdk.internal.database.query.where
import org.matrix.android.sdk.internal.di.SessionDatabase
import org.matrix.android.sdk.internal.di.UserId
Expand Down Expand Up @@ -80,7 +81,8 @@ 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!

existingChunk.fixChunkLinks(realm, roomId, direction, prevToken, nextToken)
return@awaitTransaction
}
val prevChunk = ChunkEntity.find(realm, roomId, nextToken = prevToken)
Expand All @@ -89,8 +91,14 @@ internal class TokenChunkEventPersistor @Inject constructor(
this.nextChunk = nextChunk
this.prevChunk = prevChunk
}
nextChunk?.prevChunk = currentChunk
prevChunk?.nextChunk = currentChunk
val allNextChunks = ChunkEntity.findAll(realm, roomId, prevToken = nextToken)
val allPrevChunks = ChunkEntity.findAll(realm, roomId, nextToken = prevToken)
allNextChunks?.forEach {
it.prevChunk = currentChunk
}
allPrevChunks?.forEach {
it.nextChunk = currentChunk
}
if (receivedChunk.events.isEmpty() && !receivedChunk.hasMore()) {
handleReachEnd(roomId, direction, currentChunk)
} else {
Expand All @@ -109,6 +117,34 @@ internal class TokenChunkEventPersistor @Inject constructor(
}
}

private fun ChunkEntity.fixChunkLinks(
realm: Realm,
roomId: String,
direction: PaginationDirection,
prevToken: String?,
nextToken: String?,
) {
if (direction == PaginationDirection.FORWARDS) {
val prevChunks = ChunkEntity.findAll(realm, roomId, nextToken = prevToken)
Timber.v("Found ${prevChunks?.size} prevChunks")
prevChunks?.forEach {
if (it.nextChunk != this) {
Timber.i("Set nextChunk for ${it.identifier()} from ${it.nextChunk?.identifier()} to ${identifier()}")
it.nextChunk = this
}
}
} else {
val nextChunks = ChunkEntity.findAll(realm, roomId, prevToken = nextToken)
Timber.v("Found ${nextChunks?.size} nextChunks")
nextChunks?.forEach {
if (it.prevChunk != this) {
Timber.i("Set prevChunk for ${it.identifier()} from ${it.prevChunk?.identifier()} to ${identifier()}")
it.prevChunk = this
}
}
}
}

private fun handleReachEnd(roomId: String, direction: PaginationDirection, currentChunk: ChunkEntity) {
Timber.v("Reach end of $roomId")
if (direction == PaginationDirection.FORWARDS) {
Expand Down