From db0d00c9981d1e0f154ea09e69e687089856f90b Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Fri, 24 Jan 2025 17:57:23 +0100 Subject: [PATCH 1/2] fix: Ensure coroutine cancellations propograte, rethrow CancellationException Previous code had legacy `try ... catch` blocks that could catch all exceptions, including `CancellationException`, thrown if the job of a suspending function is cancelled. Indiscriminately catching those can interfere with cancellation, so use `currentCoroutineContext().ensureActive()` to rethrow the exception if the job has been cancelled. --- .../components/account/media/AccountMediaRemoteMediator.kt | 3 +++ .../components/conversation/ConversationsRemoteMediator.kt | 3 +++ .../components/conversation/ConversationsViewModel.kt | 4 ++++ .../components/followedtags/FollowedTagsRemoteMediator.kt | 3 +++ .../instancemute/fragment/InstanceListFragment.kt | 3 +++ .../pachli/components/notifications/NotificationFetcher.kt | 4 ++++ .../components/notifications/NotificationsViewModel.kt | 4 ++++ .../components/report/adapter/StatusesPagingSource.kt | 3 +++ .../timeline/viewmodel/CachedTimelineRemoteMediator.kt | 3 +++ .../timeline/viewmodel/NetworkTimelineRemoteMediator.kt | 3 +++ .../components/timeline/viewmodel/TimelineViewModel.kt | 3 +++ .../app/pachli/components/viewthread/ViewThreadViewModel.kt | 6 ++++++ app/src/main/java/app/pachli/service/SendStatusService.kt | 3 +++ .../java/app/pachli/worker/PruneLogEntryEntityWorker.kt | 3 +++ .../repository/notifications/NotificationsRemoteMediator.kt | 3 +++ 15 files changed, 51 insertions(+) diff --git a/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt b/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt index 226de8ee55..27371d0eff 100644 --- a/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/account/media/AccountMediaRemoteMediator.kt @@ -23,6 +23,8 @@ import androidx.paging.RemoteMediator import app.pachli.core.database.model.AccountEntity import app.pachli.core.navigation.AttachmentViewData import app.pachli.core.network.retrofit.MastodonApi +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import retrofit2.HttpException @OptIn(ExperimentalPagingApi::class) @@ -71,6 +73,7 @@ class AccountMediaRemoteMediator( viewModel.currentSource?.invalidate() return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty()) } catch (e: Exception) { + currentCoroutineContext().ensureActive() return MediatorResult.Error(e) } } diff --git a/app/src/main/java/app/pachli/components/conversation/ConversationsRemoteMediator.kt b/app/src/main/java/app/pachli/components/conversation/ConversationsRemoteMediator.kt index 4474afc364..823fe18a9c 100644 --- a/app/src/main/java/app/pachli/components/conversation/ConversationsRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/conversation/ConversationsRemoteMediator.kt @@ -10,6 +10,8 @@ import app.pachli.core.database.di.TransactionProvider import app.pachli.core.database.model.ConversationEntity import app.pachli.core.network.model.HttpHeaderLink import app.pachli.core.network.retrofit.MastodonApi +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import retrofit2.HttpException @OptIn(ExperimentalPagingApi::class) @@ -73,6 +75,7 @@ class ConversationsRemoteMediator( } return MediatorResult.Success(endOfPaginationReached = nextKey == null) } catch (e: Exception) { + currentCoroutineContext().ensureActive() return MediatorResult.Error(e) } } diff --git a/app/src/main/java/app/pachli/components/conversation/ConversationsViewModel.kt b/app/src/main/java/app/pachli/components/conversation/ConversationsViewModel.kt index dc58805a73..ac7d272eeb 100644 --- a/app/src/main/java/app/pachli/components/conversation/ConversationsViewModel.kt +++ b/app/src/main/java/app/pachli/components/conversation/ConversationsViewModel.kt @@ -36,6 +36,8 @@ import app.pachli.usecase.TimelineCases import at.connyduck.calladapter.networkresult.fold import dagger.hilt.android.lifecycle.HiltViewModel import javax.inject.Inject +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.filterIsInstance @@ -181,6 +183,7 @@ class ConversationsViewModel @Inject constructor( accountId = accountManager.activeAccount!!.id, ) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.w(e, "failed to delete conversation") } } @@ -197,6 +200,7 @@ class ConversationsViewModel @Inject constructor( muted, ) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.w(e, "failed to mute conversation") } } diff --git a/app/src/main/java/app/pachli/components/followedtags/FollowedTagsRemoteMediator.kt b/app/src/main/java/app/pachli/components/followedtags/FollowedTagsRemoteMediator.kt index 073a986122..3e5068e653 100644 --- a/app/src/main/java/app/pachli/components/followedtags/FollowedTagsRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/followedtags/FollowedTagsRemoteMediator.kt @@ -7,6 +7,8 @@ import androidx.paging.RemoteMediator import app.pachli.core.network.model.HashTag import app.pachli.core.network.model.HttpHeaderLink import app.pachli.core.network.retrofit.MastodonApi +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import retrofit2.HttpException import retrofit2.Response @@ -25,6 +27,7 @@ class FollowedTagsRemoteMediator( return applyResponse(response) } catch (e: Exception) { + currentCoroutineContext().ensureActive() MediatorResult.Error(e) } } diff --git a/app/src/main/java/app/pachli/components/instancemute/fragment/InstanceListFragment.kt b/app/src/main/java/app/pachli/components/instancemute/fragment/InstanceListFragment.kt index 0a2411cd9e..c61ca75b0a 100644 --- a/app/src/main/java/app/pachli/components/instancemute/fragment/InstanceListFragment.kt +++ b/app/src/main/java/app/pachli/components/instancemute/fragment/InstanceListFragment.kt @@ -23,6 +23,8 @@ import com.google.android.material.divider.MaterialDividerItemDecoration import com.google.android.material.snackbar.Snackbar import dagger.hilt.android.AndroidEntryPoint import javax.inject.Inject +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import timber.log.Timber @@ -110,6 +112,7 @@ class InstanceListFragment : onFetchInstancesFailure(Exception(response.message())) } } catch (e: Exception) { + currentCoroutineContext().ensureActive() onFetchInstancesFailure(e) } } diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt index a94c3893ea..28a49876ca 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt @@ -43,7 +43,9 @@ import javax.inject.Inject import kotlin.collections.set import kotlin.math.min import kotlin.time.Duration.Companion.milliseconds +import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.take import timber.log.Timber @@ -144,6 +146,7 @@ class NotificationFetcher @Inject constructor( entity, ) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "Error while fetching notifications") } } @@ -248,6 +251,7 @@ class NotificationFetcher @Inject constructor( Timber.d("Fetched marker for %s: %s", account.fullName, notificationMarker) notificationMarker } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "Failed to fetch marker") null } diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt b/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt index c94829f16d..6c50b2a52d 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationsViewModel.kt @@ -62,6 +62,8 @@ import dagger.assisted.AssistedInject import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharingStarted @@ -462,6 +464,7 @@ class NotificationsViewModel @AssistedInject constructor( } Ok(NotificationActionSuccess.from(action)) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Err(UiError.make(e, action)) } _uiResult.send(result) @@ -583,6 +586,7 @@ class NotificationsViewModel @AssistedInject constructor( if (!isSuccessful) _uiResult.send(Err(UiError.make(HttpException(this), action))) } } catch (e: Exception) { + currentCoroutineContext().ensureActive() _uiResult.send(Err(UiError.make(e, action))) } } diff --git a/app/src/main/java/app/pachli/components/report/adapter/StatusesPagingSource.kt b/app/src/main/java/app/pachli/components/report/adapter/StatusesPagingSource.kt index 7bb337f06a..d23aad23a9 100644 --- a/app/src/main/java/app/pachli/components/report/adapter/StatusesPagingSource.kt +++ b/app/src/main/java/app/pachli/components/report/adapter/StatusesPagingSource.kt @@ -22,6 +22,8 @@ import app.pachli.core.network.model.Status import app.pachli.core.network.retrofit.MastodonApi import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.withContext import timber.log.Timber @@ -69,6 +71,7 @@ class StatusesPagingSource( nextKey = result.lastOrNull()?.id, ) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.w(e, "failed to load statuses") return LoadResult.Error(e) } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt index ed20cf3837..5bb327072f 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt @@ -34,6 +34,8 @@ import app.pachli.core.network.model.Status import app.pachli.core.network.retrofit.MastodonApi import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import okhttp3.Headers import retrofit2.HttpException import retrofit2.Response @@ -159,6 +161,7 @@ class CachedTimelineRemoteMediator( return MediatorResult.Success(endOfPaginationReached = false) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "Error loading, LoadType = %s", loadType) MediatorResult.Error(e) } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt index 7a6df7b956..44d57001a2 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt @@ -28,6 +28,8 @@ import app.pachli.core.model.Timeline import app.pachli.core.network.model.Status import app.pachli.core.network.retrofit.MastodonApi import java.io.IOException +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import retrofit2.HttpException import retrofit2.Response import timber.log.Timber @@ -97,6 +99,7 @@ class NetworkTimelineRemoteMediator( return MediatorResult.Success(endOfPaginationReached = endOfPaginationReached) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "Error loading, LoadType = %s", loadType) MediatorResult.Error(e) } diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt index 59a3b6a1e6..347df8de32 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/TimelineViewModel.kt @@ -62,6 +62,8 @@ import com.github.michaelbull.result.Err import com.github.michaelbull.result.Ok import com.github.michaelbull.result.Result import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharingStarted @@ -375,6 +377,7 @@ abstract class TimelineViewModel( // Result<_, _> instead of NetworkResult. _uiResult.send(Ok(StatusActionSuccess.from(action))) } catch (e: Exception) { + currentCoroutineContext().ensureActive() _uiResult.send(Err(UiError.make(e, action))) } } diff --git a/app/src/main/java/app/pachli/components/viewthread/ViewThreadViewModel.kt b/app/src/main/java/app/pachli/components/viewthread/ViewThreadViewModel.kt index 2ab1f73d72..ea41c06e8c 100644 --- a/app/src/main/java/app/pachli/components/viewthread/ViewThreadViewModel.kt +++ b/app/src/main/java/app/pachli/components/viewthread/ViewThreadViewModel.kt @@ -53,6 +53,8 @@ import javax.inject.Inject import kotlinx.coroutines.Job import kotlinx.coroutines.async import kotlinx.coroutines.channels.BufferOverflow +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow @@ -277,6 +279,7 @@ class ViewThreadViewModel @Inject constructor( try { timelineCases.reblog(status.actionableId, reblog).getOrThrow() } catch (t: Exception) { + currentCoroutineContext().ensureActive() ifExpected(t) { Timber.d(t, "Failed to reblog status: %s", status.actionableId) } @@ -287,6 +290,7 @@ class ViewThreadViewModel @Inject constructor( try { timelineCases.favourite(status.actionableId, favorite).getOrThrow() } catch (t: Exception) { + currentCoroutineContext().ensureActive() ifExpected(t) { Timber.d(t, "Failed to favourite status: %s ", status.actionableId) } @@ -297,6 +301,7 @@ class ViewThreadViewModel @Inject constructor( try { timelineCases.bookmark(status.actionableId, bookmark).getOrThrow() } catch (t: Exception) { + currentCoroutineContext().ensureActive() ifExpected(t) { Timber.d(t, "Failed to bookmark status: %s", status.actionableId) } @@ -312,6 +317,7 @@ class ViewThreadViewModel @Inject constructor( try { timelineCases.voteInPoll(status.actionableId, poll.id, choices).getOrThrow() } catch (t: Exception) { + currentCoroutineContext().ensureActive() ifExpected(t) { Timber.d(t, "Failed to vote in poll: %s", status.actionableId) } diff --git a/app/src/main/java/app/pachli/service/SendStatusService.kt b/app/src/main/java/app/pachli/service/SendStatusService.kt index 339ab00c68..4339be8588 100644 --- a/app/src/main/java/app/pachli/service/SendStatusService.kt +++ b/app/src/main/java/app/pachli/service/SendStatusService.kt @@ -50,7 +50,9 @@ import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.parcelize.Parcelize @@ -179,6 +181,7 @@ class SendStatusService : Service() { mediaCheckRetries++ } } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.w(e, "failed getting media status") retrySending(statusId) return@launch diff --git a/app/src/main/java/app/pachli/worker/PruneLogEntryEntityWorker.kt b/app/src/main/java/app/pachli/worker/PruneLogEntryEntityWorker.kt index a857a03649..5607eea980 100644 --- a/app/src/main/java/app/pachli/worker/PruneLogEntryEntityWorker.kt +++ b/app/src/main/java/app/pachli/worker/PruneLogEntryEntityWorker.kt @@ -31,6 +31,8 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import java.time.Instant import kotlin.time.Duration.Companion.hours +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import timber.log.Timber /** Prune the database cache of old statuses. */ @@ -49,6 +51,7 @@ class PruneLogEntryEntityWorker @AssistedInject constructor( logEntryDao.prune(oldest) Result.success() } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "error in PruneLogEntryEntityWorker.doWork") Result.failure() } diff --git a/core/data/src/main/kotlin/app/pachli/core/data/repository/notifications/NotificationsRemoteMediator.kt b/core/data/src/main/kotlin/app/pachli/core/data/repository/notifications/NotificationsRemoteMediator.kt index f5256fec0f..60fab301ba 100644 --- a/core/data/src/main/kotlin/app/pachli/core/data/repository/notifications/NotificationsRemoteMediator.kt +++ b/core/data/src/main/kotlin/app/pachli/core/data/repository/notifications/NotificationsRemoteMediator.kt @@ -44,6 +44,8 @@ import app.pachli.core.network.model.TimelineAccount import app.pachli.core.network.retrofit.MastodonApi import kotlinx.coroutines.async import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive import okhttp3.Headers import retrofit2.HttpException import retrofit2.Response @@ -156,6 +158,7 @@ class NotificationsRemoteMediator( MediatorResult.Success(endOfPaginationReached = false) } catch (e: Exception) { + currentCoroutineContext().ensureActive() Timber.e(e, "error loading, loadtype = %s", loadType) MediatorResult.Error(e) } From 85da328afad4d2efe69641df20678490299e7bad Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Fri, 24 Jan 2025 17:59:59 +0100 Subject: [PATCH 2/2] chore: Log after updating the notification marker Suspect a crash is occurring in the database update on the preceding line. Log afterwards; if the log entry is missing the crash location is confirmed. --- .../app/pachli/components/notifications/NotificationFetcher.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt index 28a49876ca..da5efd9a10 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationFetcher.kt @@ -235,6 +235,7 @@ class NotificationFetcher @Inject constructor( notificationsLastReadId = newMarkerId, ) accountManager.setNotificationMarkerId(account.id, newMarkerId) + Timber.d("Updated notification marker for %s to: %s", account.fullName, newMarkerId) } return notifications