From ca9e4bda9f39d5a9fd20887696914b98a0617e44 Mon Sep 17 00:00:00 2001 From: Shobhit Agarwal Date: Wed, 27 Nov 2024 00:28:03 +0530 Subject: [PATCH] Adds exception handling for coroutine cancellation when using await() (#2869) * Don't switch context when converting Lois as it already using IO dispatcher * Add try-catch handling for await() usages in FirestoreDataStore --------- Co-authored-by: Gino Miceli <228050+gino-m@users.noreply.github.com> --- config/detekt/detekt.yml | 1 + .../remote/firebase/FirebaseStorageManager.kt | 8 +++- .../remote/firebase/FirestoreDataStore.kt | 21 +++++++--- .../base/FluentCollectionReference.kt | 8 +--- .../firebase/schema/JobCollectionReference.kt | 9 ++++- .../firebase/schema/LoiCollectionReference.kt | 38 ++++++++++--------- .../schema/SurveyDocumentReference.kt | 19 +++++++--- .../schema/TermsOfServiceDocumentReference.kt | 11 +++++- 8 files changed, 74 insertions(+), 41 deletions(-) diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 0ac87eef03..481da8d551 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -273,6 +273,7 @@ exceptions: SwallowedException: active: true ignoredExceptionTypes: + - 'CancellationException' - 'InterruptedException' - 'MalformedURLException' - 'NumberFormatException' diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt index ba1bacaa94..147817b28b 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirebaseStorageManager.kt @@ -22,7 +22,9 @@ import java.io.File import java.util.StringJoiner import javax.inject.Inject import javax.inject.Singleton +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.tasks.await +import timber.log.Timber // TODO: Add column to Submission table for storing uploaded media urls // TODO: Synced to remote db as well @@ -42,7 +44,11 @@ class FirebaseStorageManager @Inject constructor() : RemoteStorageManager { // Do not delete the file after successful upload. It is used as a cache // while viewing submissions when network is unavailable. override suspend fun uploadMediaFromFile(file: File, remoteDestinationPath: String) { - createReference(remoteDestinationPath).putFile(Uri.fromFile(file)).await() + try { + createReference(remoteDestinationPath).putFile(Uri.fromFile(file)).await() + } catch (e: CancellationException) { + Timber.i(e, "Uploading media to remote storage cancelled") + } } companion object { diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt index db072ec193..4bf14c31ce 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/FirestoreDataStore.kt @@ -32,6 +32,7 @@ import com.google.firebase.ktx.Firebase import com.google.firebase.messaging.ktx.messaging import javax.inject.Inject import javax.inject.Singleton +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.emitAll @@ -54,7 +55,7 @@ internal constructor( private suspend fun db() = GroundFirestore(firestoreProvider.get()) - override suspend fun loadSurvey(surveyId: String): Survey = + override suspend fun loadSurvey(surveyId: String): Survey? = withContext(ioDispatcher) { db().surveys().survey(surveyId).get() } override suspend fun loadTermsOfService(): TermsOfService? = @@ -70,14 +71,20 @@ internal constructor( } override suspend fun loadPredefinedLois(survey: Survey) = - db().surveys().survey(survey.id).lois().fetchPredefined(survey) + withContext(ioDispatcher) { db().surveys().survey(survey.id).lois().fetchPredefined(survey) } override suspend fun loadUserLois(survey: Survey, ownerUserId: String) = - db().surveys().survey(survey.id).lois().fetchUserDefined(survey, ownerUserId) + withContext(ioDispatcher) { + db().surveys().survey(survey.id).lois().fetchUserDefined(survey, ownerUserId) + } override suspend fun subscribeToSurveyUpdates(surveyId: String) { Timber.d("Subscribing to FCM topic $surveyId") - Firebase.messaging.subscribeToTopic(surveyId).await() + try { + Firebase.messaging.subscribeToTopic(surveyId).await() + } catch (e: CancellationException) { + Timber.i(e, "Subscribing to FCM topic was cancelled") + } } /** @@ -85,7 +92,11 @@ internal constructor( * network is available. */ override suspend fun refreshUserProfile() { - firebaseFunctions.getHttpsCallable(PROFILE_REFRESH_CLOUD_FUNCTION_NAME).call().await() + try { + firebaseFunctions.getHttpsCallable(PROFILE_REFRESH_CLOUD_FUNCTION_NAME).call().await() + } catch (e: CancellationException) { + Timber.i(e, "Calling profile refresh function was cancelled") + } } override suspend fun applyMutations(mutations: List, user: User) { diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/base/FluentCollectionReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/base/FluentCollectionReference.kt index dc9555d3fd..35005d9efc 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/base/FluentCollectionReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/base/FluentCollectionReference.kt @@ -17,15 +17,9 @@ package com.google.android.ground.persistence.remote.firebase.base import com.google.firebase.firestore.CollectionReference -import kotlinx.coroutines.CoroutineDispatcher -import kotlinx.coroutines.Dispatchers open class FluentCollectionReference -protected constructor( - private val reference: CollectionReference, - protected val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, - protected val defaultDispatcher: CoroutineDispatcher = Dispatchers.Default, -) { +protected constructor(private val reference: CollectionReference) { protected fun reference(): CollectionReference = reference diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt index 4a7ca00470..478a8ab8e3 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobCollectionReference.kt @@ -19,14 +19,19 @@ package com.google.android.ground.persistence.remote.firebase.schema import com.google.android.ground.model.job.Job import com.google.android.ground.persistence.remote.firebase.base.FluentCollectionReference import com.google.firebase.firestore.CollectionReference +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.tasks.await -import kotlinx.coroutines.withContext +import timber.log.Timber class JobCollectionReference internal constructor(ref: CollectionReference) : FluentCollectionReference(ref) { + suspend fun get(): List = - withContext(ioDispatcher) { + try { val docs = reference().get().await() docs.map { doc -> JobConverter.toJob(doc) } + } catch (e: CancellationException) { + Timber.i(e, "Fetching Jobs was cancelled") + listOf() } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/LoiCollectionReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/LoiCollectionReference.kt index a37c9341a7..f2cb4e5c12 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/LoiCollectionReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/LoiCollectionReference.kt @@ -22,9 +22,9 @@ import com.google.android.ground.persistence.remote.firebase.base.FluentCollecti import com.google.android.ground.persistence.remote.firebase.schema.LoiConverter.toLoi import com.google.android.ground.proto.LocationOfInterest as LocationOfInterestProto import com.google.firebase.firestore.CollectionReference -import com.google.firebase.firestore.QuerySnapshot +import com.google.firebase.firestore.Query +import kotlin.coroutines.cancellation.CancellationException import kotlinx.coroutines.tasks.await -import kotlinx.coroutines.withContext import timber.log.Timber /** @@ -42,30 +42,32 @@ class LoiCollectionReference internal constructor(ref: CollectionReference) : /** Retrieves all "predefined" LOIs in the specified survey. Main-safe. */ suspend fun fetchPredefined(survey: Survey): List = - withContext(ioDispatcher) { - // Use !=false rather than ==true to not break legacy dev surveys. - // TODO(#2375): Switch to whereEqualTo(true) once legacy dev surveys deleted or migrated. - val query = - reference().whereEqualTo(SOURCE_FIELD, LocationOfInterestProto.Source.IMPORTED.number) - toLois(survey, query.get().await()) - } + // Use !=false rather than ==true to not break legacy dev surveys. + // TODO(#2375): Switch to whereEqualTo(true) once legacy dev surveys deleted or migrated. + fetchLois( + survey, + reference().whereEqualTo(SOURCE_FIELD, LocationOfInterestProto.Source.IMPORTED.number), + ) /** Retrieves LOIs created by the specified email in the specified survey. Main-safe. */ suspend fun fetchUserDefined(survey: Survey, ownerUserId: String): List = - withContext(ioDispatcher) { - val query = - reference() - .whereEqualTo(SOURCE_FIELD, LocationOfInterestProto.Source.FIELD_DATA.number) - .whereEqualTo(OWNER_FIELD, ownerUserId) - toLois(survey, query.get().await()) - } + fetchLois( + survey, + reference() + .whereEqualTo(SOURCE_FIELD, LocationOfInterestProto.Source.FIELD_DATA.number) + .whereEqualTo(OWNER_FIELD, ownerUserId), + ) - private suspend fun toLois(survey: Survey, snapshot: QuerySnapshot): List = - withContext(defaultDispatcher) { + private suspend fun fetchLois(survey: Survey, query: Query): List = + try { + val snapshot = query.get().await() snapshot.documents.mapNotNull { toLoi(survey, it) .onFailure { t -> Timber.e(t, "Invalid LOI ${it.id} in remote survey ${survey.id}") } .getOrNull() } + } catch (e: CancellationException) { + Timber.i(e, "Fetching LOIs was cancelled") + listOf() } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt index 283659c0eb..909c2b0a95 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/SurveyDocumentReference.kt @@ -19,7 +19,9 @@ package com.google.android.ground.persistence.remote.firebase.schema import com.google.android.ground.model.Survey import com.google.android.ground.persistence.remote.firebase.base.FluentDocumentReference import com.google.firebase.firestore.DocumentReference +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.tasks.await +import timber.log.Timber private const val LOIS = "lois" private const val SUBMISSIONS = "submissions" @@ -34,11 +36,16 @@ class SurveyDocumentReference internal constructor(ref: DocumentReference) : private fun jobs() = JobCollectionReference(reference().collection(JOBS)) - suspend fun get(): Survey { - val surveyDoc = reference().get().await() - // TODO(https://github.com/google/ground-android/issues/2864): Move jobs fetch to outside this - // DocumentReference class. - val jobs = jobs().get() - return SurveyConverter.toSurvey(surveyDoc, jobs) + suspend fun get(): Survey? { + try { + val surveyDoc = reference().get().await() + // TODO(https://github.com/google/ground-android/issues/2864): Move jobs fetch to outside this + // DocumentReference class. + val jobs = jobs().get() + return SurveyConverter.toSurvey(surveyDoc, jobs) + } catch (e: CancellationException) { + Timber.i(e, "Fetching survey was cancelled") + return null + } } } diff --git a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/TermsOfServiceDocumentReference.kt b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/TermsOfServiceDocumentReference.kt index 730e2bb831..abfe2a1bba 100644 --- a/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/TermsOfServiceDocumentReference.kt +++ b/ground/src/main/java/com/google/android/ground/persistence/remote/firebase/schema/TermsOfServiceDocumentReference.kt @@ -19,7 +19,9 @@ package com.google.android.ground.persistence.remote.firebase.schema import com.google.android.ground.model.TermsOfService import com.google.android.ground.persistence.remote.firebase.base.FluentDocumentReference import com.google.firebase.firestore.DocumentReference +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.tasks.await +import timber.log.Timber class TermsOfServiceDocumentReference internal constructor(ref: DocumentReference) : FluentDocumentReference(ref) { @@ -27,7 +29,12 @@ class TermsOfServiceDocumentReference internal constructor(ref: DocumentReferenc fun terms() = TermsOfServiceDocumentReference(reference()) suspend fun get(): TermsOfService? { - val documentSnapshot = reference().get().await() - return TermsOfServiceConverter.toTerms(documentSnapshot) + try { + val documentSnapshot = reference().get().await() + return TermsOfServiceConverter.toTerms(documentSnapshot) + } catch (e: CancellationException) { + Timber.i(e, "Fetching TermsOfService was cancelled") + return null + } } }