Skip to content

Commit

Permalink
Adds exception handling for coroutine cancellation when using await() (
Browse files Browse the repository at this point in the history
…#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>
  • Loading branch information
shobhitagarwal1612 and gino-m authored Nov 26, 2024
1 parent 981478b commit ca9e4bd
Show file tree
Hide file tree
Showing 8 changed files with 74 additions and 41 deletions.
1 change: 1 addition & 0 deletions config/detekt/detekt.yml
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ exceptions:
SwallowedException:
active: true
ignoredExceptionTypes:
- 'CancellationException'
- 'InterruptedException'
- 'MalformedURLException'
- 'NumberFormatException'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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? =
Expand All @@ -70,22 +71,32 @@ 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")
}
}

/**
* Calls Cloud Function to refresh the current user's profile info in the remote database if
* 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<Mutation>, user: User) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Job> =
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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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<LocationOfInterest> =
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<LocationOfInterest> =
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<LocationOfInterest> =
withContext(defaultDispatcher) {
private suspend fun fetchLois(survey: Survey, query: Query): List<LocationOfInterest> =
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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,22 @@ 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) {

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
}
}
}

0 comments on commit ca9e4bd

Please sign in to comment.