Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -213,32 +213,33 @@ class InstancesDataService(
}
}

fun sendInstances(
suspend fun sendInstances(
projectId: String,
instances: List<Instance>,
referrer: String,
overrideURL: String?,
cancelAfterAuthException: Boolean,
externalDeleteAfterUpload: Boolean?,
defaultSuccessMessage: String,
ensureActive: () -> Unit,
onProgress: (current: Int, total: Int) -> Unit = { _, _ -> }
): List<InstanceUploadResult> {
val projectDependencyModule = projectDependencyModuleFactory.create(projectId)

return projectDependencyModule.instancesLock.withLock { acquiredLock: Boolean ->
return projectDependencyModule.instancesLock.withLockSuspend { acquiredLock: Boolean ->
if (acquiredLock) {
instanceSubmitter.submitInstances(
val result = instanceSubmitter.submitInstances(
projectId,
instances,
referrer,
overrideURL,
cancelAfterAuthException,
externalDeleteAfterUpload,
defaultSuccessMessage,
ensureActive,
onProgress
)
update(projectId)

result
} else {
emptyList()
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.odk.collect.android.instancemanagement.send

import kotlinx.coroutines.currentCoroutineContext
import kotlinx.coroutines.ensureActive
import kotlinx.coroutines.runInterruptible
import org.odk.collect.android.instancemanagement.InstanceDeleter
import org.odk.collect.android.projects.ProjectDependencyModule
import org.odk.collect.android.utilities.InstanceAutoDeleteChecker
Expand All @@ -20,15 +23,46 @@ class InstanceSubmitter(
) {

fun submitInstances(
projectId: String,
toUpload: List<Instance>
): List<InstanceUploadResult> {
val projectDependencyModule = projectDependencyFactory.create(projectId)
val formsRepository = projectDependencyModule.formsRepository
val instancesRepository = projectDependencyModule.instancesRepository
val generalSettings = projectDependencyModule.generalSettings

val uploadResults = mutableListOf<InstanceUploadResult>()
val deviceId = propertyManager.getSingularProperty(PROPMGR_DEVICE_ID)

val sortedInstances = toUpload.sortedBy { it.finalizationDate }
for (instance in sortedInstances) {
try {
val resultMessage = instanceUploader.uploadOneSubmission(projectId, instance, deviceId, null, "")
uploadResults.add(InstanceUploadResult.Success(instance, resultMessage))

deleteInstance(instance, formsRepository, instancesRepository, generalSettings, null)
} catch (e: FormUploadException) {
Timber.d(e)
uploadResults.add(InstanceUploadResult.Error(instance, e))

if (e is FormUploadAuthRequestedException) {
break
}
}
}

return uploadResults
}

suspend fun submitInstances(
projectId: String,
toUpload: List<Instance>,
referrer: String = "",
overrideURL: String? = null,
cancelAfterAuthException: Boolean = false,
externalDeleteAfterUpload: Boolean? = null,
defaultSuccessMessage: String? = null,
ensureActive: () -> Unit = {},
onProgress: (current: Int, total: Int) -> Unit = { _, _ -> }
referrer: String,
overrideURL: String?,
cancelAfterAuthException: Boolean,
externalDeleteAfterUpload: Boolean?,
defaultSuccessMessage: String?,
onProgress: (current: Int, total: Int) -> Unit
): List<InstanceUploadResult> {
val projectDependencyModule = projectDependencyFactory.create(projectId)
val formsRepository = projectDependencyModule.formsRepository
Expand All @@ -40,13 +74,14 @@ class InstanceSubmitter(

val sortedInstances = toUpload.sortedBy { it.finalizationDate }
for ((index, instance) in sortedInstances.withIndex()) {
ensureActive()
onProgress( index + 1, sortedInstances.size)
currentCoroutineContext().ensureActive()
onProgress(index + 1, sortedInstances.size)

try {
val resultMessage = instanceUploader.uploadOneSubmission(projectId, instance, deviceId, overrideURL, referrer)
val resultMessage = runInterruptible {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the old process allowed thread interruption I'm guessing this will work, but it still scares me a bit. Is there an exception catch that's happening within the upload code that's handling the InterruptedException that should get thrown?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it happens in OpenRosaServerInstanceUploader#uploadOneSubmission. It worked in the same way with the old async task.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing you just mean the blanket Exception catches like here? This still feels like a problem as a CancellationException (the exception that runInterruptible uses instead of InterruptedException) can be thrown in any window where the coroutine "blocks". As far as I'm aware this includes any I/O (network, disk etc.), so there are definitely opportunities for interrupts in places we're not handling it like with DB accesses. This was probably also a problem with the old code, but I don't think it's a problem we should port over to this rework.

I'd prefer opting for a more explicit cancellation mechanism like we use in ServerFormUseCases#downloadForms. This lets us reason about exactly where cancellation can occur and ensure we recover from it correctly.

instanceUploader.uploadOneSubmission(projectId, instance, deviceId, overrideURL, referrer)
}
uploadResults.add(InstanceUploadResult.Success(instance, resultMessage ?: defaultSuccessMessage))

deleteInstance(instance, formsRepository, instancesRepository, generalSettings, externalDeleteAfterUpload)
} catch (e: FormUploadException) {
Timber.d(e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import androidx.lifecycle.MutableLiveData
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.ensureActive
import kotlinx.coroutines.launch
import org.odk.collect.android.instancemanagement.InstancesDataService
import org.odk.collect.android.utilities.WebCredentialsUtils
Expand Down Expand Up @@ -55,7 +54,6 @@ class InstanceUploadViewModel(
true,
externalDeleteAfterUpload,
defaultSuccessMessage,
{ coroutineContext.ensureActive() }
) { current, total ->
_state.postValue(UploadState.Progress(current, total))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,99 @@ class InstanceUploadViewModelTest {
private lateinit var viewModel: InstanceUploadViewModel

@Test
fun `submitted instance is deleted even when upload is cancelled`() {
fun `instance is not submitted when upload process is canceled during execution`() {
val form = FormFixtures.form("1")
val formsRepository = InMemFormsRepository().apply {
save(form)
}

val instance = Instance.Builder()
.dbId(1)
.formId(form.formId)
.formVersion(form.version)
.status(Instance.STATUS_COMPLETE)
.finalizationDate(1)
.build()

val instancesRepository = InMemInstancesRepository().apply {
save(instance)
}

val projectsDependencyModuleFactory = CachingProjectDependencyModuleFactory { projectId ->
ProjectDependencyModule(
projectId,
{ InMemSettings() },
{ formsRepository },
{ instancesRepository },
mock(),
{ ChangeLocks(BooleanChangeLock(), BooleanChangeLock()) },
mock(),
mock(),
mock(),
mock(),
mock()
)
}

val submittedInstances = mutableListOf<Long>()

val instanceUploader = object : InstanceUploader {
override fun uploadOneSubmission(
projectId: String,
instance: Instance,
deviceId: String?,
overrideURL: String?,
referrer: String
): String {
viewModel.cancel()
Thread.sleep(1000)

submittedInstances.add(instance.dbId)
instancesRepository.save(
Instance.Builder(instance)
.status(Instance.STATUS_SUBMITTED)
.build()
)
return "Success"
}
}

val instancesSubmitter = InstanceSubmitter(
instanceUploader,
projectsDependencyModuleFactory,
mock()
)
val instancesDataService = InstancesDataService(
AppState(),
mock(),
projectsDependencyModuleFactory,
mock(),
instancesSubmitter
) {}

val dispatcherProvider = TestDispatcherProvider()
viewModel = InstanceUploadViewModel(
dispatcherProvider,
mock(),
instancesRepository,
instancesDataService,
"projectId",
"",
null,
null,
null,
null,
"Success",
"Waiting"
)
viewModel.upload(listOf(instance.dbId))
dispatcherProvider.runBackground()

assertThat(submittedInstances.isEmpty(), equalTo(true))
}

@Test
fun `submitted instances are deleted when upload process is canceled during execution`() {
val form = FormFixtures.form("1")
val formsRepository = InMemFormsRepository().apply {
save(form)
Expand Down Expand Up @@ -75,6 +167,7 @@ class InstanceUploadViewModelTest {

val submittedInstances = mutableListOf<Long>()

var progress = 0;
val instanceUploader = object : InstanceUploader {
override fun uploadOneSubmission(
projectId: String,
Expand All @@ -83,13 +176,16 @@ class InstanceUploadViewModelTest {
overrideURL: String?,
referrer: String
): String {
if (++progress == 2) {
viewModel.cancel()
}

submittedInstances.add(instance.dbId)
instancesRepository.save(
Instance.Builder(instance)
.status(Instance.STATUS_SUBMITTED)
.build()
)
viewModel.cancel()
return "Success"
}
}
Expand Down
12 changes: 12 additions & 0 deletions shared/src/main/java/org/odk/collect/shared/locks/ChangeLock.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ interface ChangeLock {
}
}

suspend fun <T> withLockSuspend(function: suspend (Boolean) -> T): T {
val acquired = tryLock(DEFAULT_TOKEN)

return try {
function(acquired)
} finally {
if (acquired) {
unlock(DEFAULT_TOKEN)
}
}
}

fun tryLock(token: Any): Boolean

fun unlock(token: Any)
Expand Down