Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DB ANRs during form entry #5867

Merged
merged 30 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
582591a
Remove UI thread writes to the DB
seadowg Nov 28, 2023
3c03dbf
Add slow call for accessing readable database
seadowg Nov 28, 2023
64be103
Load BlankFormListItems off the UI thread
seadowg Nov 28, 2023
70a057e
Use Flow instead of LiveData to run map on background thread
seadowg Nov 29, 2023
f51a815
Convert TestScheduler to Kotlin
seadowg Nov 29, 2023
781c04c
Support using Scheduler for Flows
seadowg Nov 29, 2023
c506468
Fix Form equality for StateFlow
seadowg Nov 29, 2023
3eaf6e6
Add Flow support to FakeScheduler
seadowg Nov 29, 2023
16591fc
Fix test helper usage
seadowg Nov 29, 2023
629e764
Rename method
seadowg Nov 29, 2023
b3743aa
Assert form can be started on background thread
seadowg Dec 5, 2023
8a82545
Move form path to constructor
seadowg Dec 5, 2023
50749f8
Use Uri as input for FormLoaderTask
seadowg Dec 6, 2023
6631caf
Ignore broken tests for now
seadowg Dec 6, 2023
11a0fdb
Allow loading form without StrictMode violation
seadowg Dec 6, 2023
95ed0a6
Lock in current StrictMode coverage for forms db
seadowg Dec 6, 2023
b928e90
Lock in current StrictMode coverage for forms db
seadowg Dec 6, 2023
e84d3fa
Rework FormLoaderTask tests
seadowg Dec 8, 2023
87b77f0
Update and reenable remaining FormLoaderTask tests
seadowg Dec 8, 2023
aa69ee8
Add loading state to FormUriActivity
seadowg Dec 8, 2023
3ca0a73
Remove new Form and Instance fields
seadowg Dec 18, 2023
859f0c4
Remove unused field
seadowg Dec 18, 2023
aaa54b7
Move change language logic to view model
seadowg Dec 18, 2023
d314f45
Remove unneeded variable
seadowg Dec 18, 2023
bdef028
Fix param names
seadowg Jan 22, 2024
eee3ec4
Correct test description
seadowg Jan 22, 2024
4f74596
Simplify ExternalSecondaryInstanceTest
seadowg Jan 22, 2024
8d537ae
Fix case where form has been deleted
seadowg Jan 22, 2024
c60752e
Simplify boolean logic
seadowg Jan 22, 2024
f5fd65c
Fix deleted form logic
seadowg Jan 22, 2024
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 @@ -18,4 +18,13 @@ class TrackableWorker(private val scheduler: Scheduler) {
foreground.accept(result)
}
}

fun immediate(background: Runnable) {
immediate(
background = {
background.run()
},
foreground = {}
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,18 @@ fun <T> LiveData<T>.getOrAwaitValue(
): T {
var data: T? = null
val latch = CountDownLatch(1)
val observer = object : Observer<T> {
override fun onChanged(o: T) {
data = o
latch.countDown()
this@getOrAwaitValue.removeObserver(this)
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't work with cases where there was already a value set on the LiveData.

}
val observer = Observer<T> { o ->
data = o
latch.countDown()
}
this.observeForever(observer)

this.observeForever(observer)
afterObserve.invoke()

// Don't wait indefinitely if the LiveData is not set.
if (!latch.await(time, timeUnit)) {
if (latch.await(time, timeUnit)) {
this.removeObserver(observer)
} else {
this.removeObserver(observer)
throw TimeoutException("LiveData value was never set.")
}
Expand Down
18 changes: 15 additions & 3 deletions async/src/main/java/org/odk/collect/async/CoroutineScheduler.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package org.odk.collect.async

import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.isActive
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
Expand All @@ -18,9 +20,15 @@ open class CoroutineScheduler(private val foregroundContext: CoroutineContext, p
}
}

override fun immediate(foreground: Runnable) {
CoroutineScope(foregroundContext).launch {
foreground.run()
override fun immediate(background: Boolean, runnable: Runnable) {
val context = if (background) {
backgroundContext
} else {
foregroundContext
}

CoroutineScope(context).launch {
runnable.run()
}
}

Expand All @@ -37,6 +45,10 @@ open class CoroutineScheduler(private val foregroundContext: CoroutineContext, p
return ScopeCancellable(repeatScope)
}

override fun <T> flowOnBackground(flow: Flow<T>): Flow<T> {
return flow.flowOn(backgroundContext)
}

override fun cancelAllDeferred() {
throw UnsupportedOperationException()
}
Expand Down
11 changes: 9 additions & 2 deletions async/src/main/java/org/odk/collect/async/Scheduler.kt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.odk.collect.async

import kotlinx.coroutines.flow.Flow
import java.util.function.Consumer
import java.util.function.Supplier

Expand All @@ -22,9 +23,9 @@ interface Scheduler {
fun <T> immediate(background: Supplier<T>, foreground: Consumer<T>)

/**
* Run work in the foreground. Cancelled if application closed.
* Run work in the foreground or background. Cancelled if application closed.
*/
fun immediate(foreground: Runnable)
fun immediate(background: Boolean = false, runnable: Runnable)

/**
* Schedule a task to run in the background even if the app isn't running. The task
Expand Down Expand Up @@ -74,4 +75,10 @@ interface Scheduler {
fun repeat(foreground: Runnable, repeatPeriod: Long): Cancellable

fun cancelAllDeferred()

fun <T> flowOnBackground(flow: Flow<T>): Flow<T>
}

fun <T> Flow<T>.flowOnBackground(scheduler: Scheduler): Flow<T> {
return scheduler.flowOnBackground(this)
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ abstract class SchedulerAsyncTaskMimic<Params, Progress, Result>(private val sch

protected fun publishProgress(vararg values: Progress) {
scheduler.immediate(
foreground = { onProgressUpdate(*values) }
runnable = { onProgressUpdate(*values) }
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import org.junit.runner.RunWith;
import org.odk.collect.android.support.rules.CollectTestRule;
import org.odk.collect.android.support.rules.TestRuleChain;
import org.odk.collect.android.support.pages.MainMenuPage;

import java.util.Collections;

Expand All @@ -32,15 +31,24 @@ public class ExternalSecondaryInstanceTest {
.around(rule);

@Test
public void displaysAllOptionsFromSecondaryInstance() {
//TestCase1
new MainMenuPage()
.copyForm("external_select_10.xml", Collections.singletonList("external_data_10.xml"))
.startBlankForm("external select 10")
.clickOnText("a")
.swipeToNextQuestion("Second")
.assertText("aa")
.assertText("ab")
.assertText("ac");
public void displaysAllOptionsFromXMLSecondaryInstance() {
rule.startAtMainMenu()
.copyForm("external_select.xml", Collections.singletonList("external_data.xml"))
.startBlankForm("external select")
.assertQuestion("First")
.assertText("One")
.assertText("Two")
.assertText("Three");
}

@Test
public void displaysAllOptionsFromCSVSecondaryInstance() {
seadowg marked this conversation as resolved.
Show resolved Hide resolved
rule.startAtMainMenu()
.copyForm("external_select_csv.xml", Collections.singletonList("external_data.csv"))
.startBlankForm("external select")
.assertQuestion("First")
.assertText("One")
.assertText("Two")
.assertText("Three");
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.odk.collect.android.feature.formentry
package org.odk.collect.android.feature.formentry.dynamicpreload

import org.junit.Rule
import org.junit.Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,6 @@ class MatchExactlyTest {
.enableMatchExactly()
.enableManualUpdates()

assertThat(testDependencies.scheduler.deferredTasks, `is`(empty()))
assertThat(testDependencies.scheduler.getDeferredTasks(), `is`(empty()))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,6 @@ class PreviouslyDownloadedOnlyTest {
.enablePreviouslyDownloadedOnlyUpdates()
.enableManualUpdates()

assertThat(testDependencies.scheduler.deferredTasks, equalTo(emptyList()))
assertThat(testDependencies.scheduler.getDeferredTasks(), equalTo(emptyList()))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import org.junit.Rule
import org.junit.Test
import org.junit.rules.RuleChain
import org.junit.runner.RunWith
import org.odk.collect.android.R
import org.odk.collect.android.support.TestDependencies
import org.odk.collect.android.support.pages.MainMenuPage
import org.odk.collect.android.support.rules.CollectTestRule
Expand All @@ -27,7 +26,7 @@ class FormManagementSettingsTest {

@Test
fun whenMatchExactlyEnabled_changingAutomaticUpdateFrequency_changesTaskFrequency() {
var deferredTasks = testDependencies.scheduler.deferredTasks
var deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks, `is`(empty()))

Expand All @@ -38,14 +37,14 @@ class FormManagementSettingsTest {
.clickUpdateForms()
.clickOption(org.odk.collect.strings.R.string.match_exactly)

deferredTasks = testDependencies.scheduler.deferredTasks
deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks.size, `is`(1))

val matchExactlyTag = deferredTasks[0].tag

page.clickAutomaticUpdateFrequency().clickOption(org.odk.collect.strings.R.string.every_one_hour)
deferredTasks = testDependencies.scheduler.deferredTasks
deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks.size, `is`(1))
assertThat(deferredTasks[0].tag, `is`(matchExactlyTag))
Expand All @@ -54,7 +53,7 @@ class FormManagementSettingsTest {

@Test
fun whenPreviouslyDownloadedOnlyEnabled_changingAutomaticUpdateFrequency_changesTaskFrequency() {
var deferredTasks = testDependencies.scheduler.deferredTasks
var deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks, `is`(empty()))

Expand All @@ -65,14 +64,14 @@ class FormManagementSettingsTest {
.clickUpdateForms()
.clickOption(org.odk.collect.strings.R.string.previously_downloaded_only)

deferredTasks = testDependencies.scheduler.deferredTasks
deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks.size, `is`(1))

val previouslyDownloadedTag = deferredTasks[0].tag
page.clickAutomaticUpdateFrequency().clickOption(org.odk.collect.strings.R.string.every_one_hour)

deferredTasks = testDependencies.scheduler.deferredTasks
deferredTasks = testDependencies.scheduler.getDeferredTasks()

assertThat(deferredTasks.size, `is`(1))
assertThat(deferredTasks[0].tag, `is`(previouslyDownloadedTag))
Expand Down
Loading