From 69bcc92c4634a3687b4e8b53e030b70ab6fd4340 Mon Sep 17 00:00:00 2001 From: Konrad Pozniak Date: Mon, 21 Feb 2022 19:33:10 +0100 Subject: [PATCH] fix cache cleanup deleting more statuses than it should (#2348) * fix cache cleanup deleting more statuses than it should * reset LOAD_AT_ONCE * improve tests * move cache clean code back to ViewModel --- .../tusky/appstore/CacheUpdater.kt | 12 +- .../viewmodel/CachedTimelineViewModel.kt | 17 +++ .../timeline/viewmodel/TimelineViewModel.kt | 1 - .../com/keylesspalace/tusky/db/TimelineDao.kt | 25 ++++- .../keylesspalace/tusky/db/TimelineDaoTest.kt | 105 +++++++++++++----- 5 files changed, 118 insertions(+), 42 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt b/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt index cb837fbc..50f96b26 100644 --- a/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt +++ b/app/src/main/java/com/keylesspalace/tusky/appstore/CacheUpdater.kt @@ -6,12 +6,11 @@ import com.keylesspalace.tusky.db.AppDatabase import io.reactivex.rxjava3.core.Single import io.reactivex.rxjava3.disposables.Disposable import io.reactivex.rxjava3.schedulers.Schedulers -import java.util.concurrent.TimeUnit import javax.inject.Inject class CacheUpdater @Inject constructor( eventHub: EventHub, - accountManager: AccountManager, + private val accountManager: AccountManager, private val appDatabase: AppDatabase, gson: Gson ) { @@ -21,11 +20,6 @@ class CacheUpdater @Inject constructor( init { val timelineDao = appDatabase.timelineDao() - Schedulers.io().scheduleDirect { - val olderThan = System.currentTimeMillis() - CLEANUP_INTERVAL - appDatabase.timelineDao().cleanup(olderThan) - } - disposable = eventHub.events.subscribe { event -> val accountId = accountManager.activeAccount?.id ?: return@subscribe when (event) { @@ -61,8 +55,4 @@ class CacheUpdater @Inject constructor( .subscribeOn(Schedulers.io()) .subscribe() } - - companion object { - val CLEANUP_INTERVAL = TimeUnit.DAYS.toMillis(14) - } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt index 1ea61693..1da37bf4 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt @@ -43,11 +43,14 @@ import com.keylesspalace.tusky.network.TimelineCases import com.keylesspalace.tusky.util.dec import com.keylesspalace.tusky.util.inc import com.keylesspalace.tusky.viewdata.StatusViewData +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import kotlinx.coroutines.rx3.await import retrofit2.HttpException import javax.inject.Inject +import kotlin.time.DurationUnit +import kotlin.time.toDuration /** * TimelineViewModel that caches all statuses in a local database @@ -81,6 +84,16 @@ class CachedTimelineViewModel @Inject constructor( } .cachedIn(viewModelScope) + init { + viewModelScope.launch { + delay(5.toDuration(DurationUnit.SECONDS)) // delay so the db is not locked during initial ui refresh + accountManager.activeAccount?.id?.let { accountId -> + db.timelineDao().cleanup(accountId, MAX_STATUSES_IN_CACHE) + db.timelineDao().cleanupAccounts(accountId) + } + } + } + override fun updatePoll(newPoll: Poll, status: StatusViewData.Concrete) { // handled by CacheUpdater } @@ -207,4 +220,8 @@ class CachedTimelineViewModel @Inject constructor( } } } + + companion object { + private const val MAX_STATUSES_IN_CACHE = 1000 + } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt index 95a31dd1..c7c95636 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt @@ -43,7 +43,6 @@ import com.keylesspalace.tusky.settings.PrefKeys import com.keylesspalace.tusky.viewdata.StatusViewData import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.launch import kotlinx.coroutines.rx3.asFlow import kotlinx.coroutines.rx3.await diff --git a/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt b/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt index 5a700027..6952ca86 100644 --- a/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt +++ b/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt @@ -98,8 +98,29 @@ AND serverId = :statusId""" ) abstract fun delete(accountId: Long, statusId: String) - @Query("""DELETE FROM TimelineStatusEntity WHERE createdAt < :olderThan""") - abstract fun cleanup(olderThan: Long) + /** + * Cleans the TimelineStatusEntity table from old status entries. + * @param accountId id of the account for which to clean statuses + * @param limit how many statuses to keep + */ + @Query( + """DELETE FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND serverId NOT IN + (SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId ORDER BY LENGTH(serverId) DESC, serverId DESC LIMIT :limit) + """ + ) + abstract suspend fun cleanup(accountId: Long, limit: Int) + + /** + * Cleans the TimelineAccountEntity table from accounts that are no longer referenced in the TimelineStatusEntity table + * @param accountId id of the user account for which to clean timeline accounts + */ + @Query( + """DELETE FROM TimelineAccountEntity WHERE timelineUserId = :accountId AND serverId NOT IN + (SELECT authorServerId FROM TimelineStatusEntity WHERE timelineUserId = :accountId) + AND serverId NOT IN + (SELECT reblogAccountId FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND reblogAccountId IS NOT NULL)""" + ) + abstract suspend fun cleanupAccounts(accountId: Long) @Query( """UPDATE TimelineStatusEntity SET poll = :poll diff --git a/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt b/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt index fa405fe9..c60f7d4f 100644 --- a/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt @@ -5,7 +5,6 @@ import androidx.room.Room import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import com.google.gson.Gson -import com.keylesspalace.tusky.appstore.CacheUpdater import com.keylesspalace.tusky.components.timeline.Placeholder import com.keylesspalace.tusky.components.timeline.toEntity import com.keylesspalace.tusky.entity.Status @@ -66,28 +65,25 @@ class TimelineDaoTest { @Test fun cleanup() = runBlocking { - val now = System.currentTimeMillis() - val oldDate = now - CacheUpdater.CLEANUP_INTERVAL - 20_000 - val oldThisAccount = makeStatus( - statusId = 5, - createdAt = oldDate - ) - val oldAnotherAccount = makeStatus( - statusId = 10, - createdAt = oldDate, - accountId = 2 - ) - val recentThisAccount = makeStatus( - statusId = 30, - createdAt = System.currentTimeMillis() - ) - val recentAnotherAccount = makeStatus( - statusId = 60, - createdAt = System.currentTimeMillis(), - accountId = 2 + + val statusesBeforeCleanup = listOf( + makeStatus(statusId = 100), + makeStatus(statusId = 10, authorServerId = "3"), + makeStatus(statusId = 8, reblog = true, authorServerId = "10"), + makeStatus(statusId = 5), + makeStatus(statusId = 3, authorServerId = "4"), + makeStatus(statusId = 2, accountId = 2, authorServerId = "5"), + makeStatus(statusId = 1, authorServerId = "5") ) - for ((status, author, reblogAuthor) in listOf(oldThisAccount, oldAnotherAccount, recentThisAccount, recentAnotherAccount)) { + val statusesAfterCleanup = listOf( + makeStatus(statusId = 100), + makeStatus(statusId = 10, authorServerId = "3"), + makeStatus(statusId = 8, reblog = true, authorServerId = "10"), + makeStatus(statusId = 2, accountId = 2, authorServerId = "5"), + ) + + for ((status, author, reblogAuthor) in statusesBeforeCleanup) { timelineDao.insertAccount(author) reblogAuthor?.let { timelineDao.insertAccount(it) @@ -95,15 +91,34 @@ class TimelineDaoTest { timelineDao.insertStatus(status) } - timelineDao.cleanup(now - CacheUpdater.CLEANUP_INTERVAL) + timelineDao.cleanup(accountId = 1, limit = 3) + timelineDao.cleanupAccounts(accountId = 1) val loadParams: PagingSource.LoadParams = PagingSource.LoadParams.Refresh(null, 100, false) - val loadedStatusAccount1 = (timelineDao.getStatusesForAccount(1).load(loadParams) as PagingSource.LoadResult.Page).data - val loadedStatusAccount2 = (timelineDao.getStatusesForAccount(2).load(loadParams) as PagingSource.LoadResult.Page).data + val loadedStatuses = (timelineDao.getStatusesForAccount(1).load(loadParams) as PagingSource.LoadResult.Page).data - assertStatuses(listOf(recentThisAccount), loadedStatusAccount1) - assertStatuses(listOf(recentAnotherAccount), loadedStatusAccount2) + assertStatuses(statusesAfterCleanup, loadedStatuses) + + val loadedAccounts: MutableList> = mutableListOf() + val accountCursor = db.query("SELECT timelineUserId, serverId FROM TimelineAccountEntity", null) + accountCursor.moveToFirst() + while (!accountCursor.isAfterLast) { + val accountId: Long = accountCursor.getLong(accountCursor.getColumnIndex("timelineUserId")) + val serverId: String = accountCursor.getString(accountCursor.getColumnIndex("serverId")) + loadedAccounts.add(accountId to serverId) + accountCursor.moveToNext() + } + + val expectedAccounts = listOf( + 1L to "3", + 1L to "10", + 1L to "R10", + 1L to "20", + 2L to "5" + ) + + assertEquals(expectedAccounts, loadedAccounts) } @Test @@ -115,8 +130,6 @@ class TimelineDaoTest { makeStatus(statusId = 1) ) - timelineDao.deleteRange(1, oldStatuses.last().first.serverId, oldStatuses.first().first.serverId) - for ((status, author, reblogAuthor) in oldStatuses) { timelineDao.insertAccount(author) reblogAuthor?.let { @@ -152,6 +165,42 @@ class TimelineDaoTest { assertStatuses(newStatuses, loadedStatuses) } + @Test + fun deleteRange() = runBlocking { + val statuses = listOf( + makeStatus(statusId = 100), + makeStatus(statusId = 15), + makeStatus(statusId = 14), + makeStatus(statusId = 13), + makeStatus(statusId = 12), + makeStatus(statusId = 11), + makeStatus(statusId = 9) + ) + + for ((status, author, reblogAuthor) in statuses) { + timelineDao.insertAccount(author) + reblogAuthor?.let { + timelineDao.insertAccount(it) + } + timelineDao.insertStatus(status) + } + + timelineDao.deleteRange(1, "12", "14") + + val pagingSource = timelineDao.getStatusesForAccount(1) + val loadResult = pagingSource.load(PagingSource.LoadParams.Refresh(null, 100, false)) + val loadedStatuses = (loadResult as PagingSource.LoadResult.Page).data + + val remainingStatuses = listOf( + makeStatus(statusId = 100), + makeStatus(statusId = 15), + makeStatus(statusId = 11), + makeStatus(statusId = 9) + ) + + assertStatuses(remainingStatuses, loadedStatuses) + } + @Test fun deleteAllForInstance() = runBlocking {