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 missing/swapped/duplicated messages due to wrong TimelineChunk modifications or insertions #5528

Merged
merged 4 commits into from
May 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/5528.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix cases of missing, swapped, or duplicated messages
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,31 @@ internal class TimelineChunk(private val chunkEntity: ChunkEntity,
private fun handleDatabaseChangeSet(results: RealmResults<TimelineEventEntity>, changeSet: OrderedCollectionChangeSet) {
val insertions = changeSet.insertionRanges
for (range in insertions) {
// Check if the insertion's displayIndices match our expectations - or skip this insertion.
// Inconsistencies (missing messages) can happen otherwise if we get insertions before having loaded all timeline events of the chunk.
if (builtEvents.isNotEmpty()) {
// Check consistency to item before insertions
if (range.startIndex > 0) {
val firstInsertion = results[range.startIndex]!!
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid !! even if we are sure it cannot be null

val lastBeforeInsertion = builtEvents[range.startIndex - 1]
Copy link
Member

Choose a reason for hiding this comment

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

If there is no built event yet, this could crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is builtEvents.isNotEmpty() in line 435

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah; I'm still wandering if we should try to fix the underneath issue here instead of doing some weird checks :/

if (firstInsertion.displayIndex + 1 != lastBeforeInsertion.displayIndex) {
Timber.i("handleDatabaseChangeSet: skip insertion at ${range.startIndex}/${builtEvents.size}, " +
"displayIndex mismatch at ${range.startIndex}: ${firstInsertion.displayIndex} -> ${lastBeforeInsertion.displayIndex}")
continue
}
}
// Check consistency to item after insertions
if (range.startIndex < builtEvents.size) {
val lastInsertion = results[range.startIndex + range.length - 1]!!
val firstAfterInsertion = builtEvents[range.startIndex]
if (firstAfterInsertion.displayIndex + 1 != lastInsertion.displayIndex) {
Timber.i("handleDatabaseChangeSet: skip insertion at ${range.startIndex}/${builtEvents.size}, " +
"displayIndex mismatch at ${range.startIndex + range.length}: " +
"${firstAfterInsertion.displayIndex} -> ${lastInsertion.displayIndex}")
continue
}
}
}
val newItems = results
.subList(range.startIndex, range.startIndex + range.length)
.map { it.buildAndDecryptIfNeeded() }
Expand All @@ -447,8 +472,12 @@ internal class TimelineChunk(private val chunkEntity: ChunkEntity,
for (range in modifications) {
for (modificationIndex in (range.startIndex until range.startIndex + range.length)) {
val updatedEntity = results[modificationIndex] ?: continue
val displayIndex = builtEventsIndexes[updatedEntity.eventId]
if (displayIndex == null) {
continue
}
try {
builtEvents[modificationIndex] = updatedEntity.buildAndDecryptIfNeeded()
builtEvents[displayIndex] = updatedEntity.buildAndDecryptIfNeeded()
} catch (failure: Throwable) {
Timber.v("Fail to update items at index: $modificationIndex")
}
Expand Down