Move unsent forms query off the main thread#3566
Conversation
This Executor discard any running task when a new task is submitted
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CompletableFuture is only supported from API 23
📝 WalkthroughWalkthroughA new Sequence DiagramsequenceDiagram
participant UI as UI/Activity
participant Executor as LatestTaskExecutor
participant BG as Background<br/>Thread
participant Handler as Main<br/>Thread
UI->>Executor: submit(task, callback)
Executor->>Executor: cancelPreviousTask()
Executor->>BG: execute task
BG->>BG: task.call()
alt Success
BG->>Handler: postResult()
Handler->>UI: callback.onResult(result)
else Exception
BG->>BG: Logger.exception()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/tasks/LatestTaskExecutor.kt`:
- Around line 22-36: The submit method can still deliver stale callbacks because
Future.cancel(true) is best‑effort; add a monotonic task id (e.g. an AtomicLong
taskCounter) and associate the id with each submitted work, storing the latest
id in a shared AtomicLong/AtomicReference (or extend currentTask to hold the
id). In LatestTaskExecutor.submit, increment the counter to produce taskId
before submitting, capture that id inside the worker, and before calling
mainHandler.post { callback.onResult(result) } verify the captured id matches
the current/latest id (skip posting if it does not). Keep the existing
cancel(previous) behavior but use the id check to guarantee only the most recent
task's result is delivered.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/tasks/LatestTaskExecutor.ktapp/src/org/commcare/utils/SyncDetailCalculations.java
| private val singleThreadExecutor: ExecutorService = Executors.newSingleThreadExecutor() | ||
| private val currentTask: AtomicReference<Future<*>?> = AtomicReference<Future<*>?>() | ||
| private val mainHandler = Handler(Looper.getMainLooper()) | ||
|
|
||
| fun submit(task: Callable<T>, callback: Callback<T>) { | ||
| val future = singleThreadExecutor.submit { | ||
| try { | ||
| val result = task.call() | ||
| mainHandler.post { callback.onResult(result) } | ||
| } catch (e: Exception) { | ||
| Logger.exception("LatestTaskExecutor task failed", e) | ||
| } | ||
| } | ||
| val previous = currentTask.getAndSet(future) | ||
| previous?.cancel(true) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find app/src/org/commcare/tasks -name "LatestTaskExecutor.kt" -type fRepository: dimagi/commcare-android
Length of output: 115
🏁 Script executed:
# Check if the file exists and read its contents
if [ -f "app/src/org/commcare/tasks/LatestTaskExecutor.kt" ]; then
wc -l "app/src/org/commcare/tasks/LatestTaskExecutor.kt"
fiRepository: dimagi/commcare-android
Length of output: 118
🏁 Script executed:
# List files in the tasks directory to understand context
fd . app/src/org/commcare/tasks --max-depth 1 2>/dev/null | head -20Repository: dimagi/commcare-android
Length of output: 1109
🏁 Script executed:
cat -n "app/src/org/commcare/tasks/LatestTaskExecutor.kt"Repository: dimagi/commcare-android
Length of output: 1649
🏁 Script executed:
# Run ktlint on the file to check formatting compliance
ktlint "app/src/org/commcare/tasks/LatestTaskExecutor.kt" 2>&1Repository: dimagi/commcare-android
Length of output: 113
🏁 Script executed:
# Search for usages of LatestTaskExecutor to understand how it's used
rg "LatestTaskExecutor" --type kt --type java -B 2 -A 5Repository: dimagi/commcare-android
Length of output: 93
🏁 Script executed:
# Also check if there are tests or other patterns that might guard against stale results
rg "submit.*task.*callback\|LatestTaskExecutor" --type kt --type java -A 10Repository: dimagi/commcare-android
Length of output: 93
🏁 Script executed:
# Search for usages of LatestTaskExecutor without type filters
rg "LatestTaskExecutor" -B 2 -A 5Repository: dimagi/commcare-android
Length of output: 2907
🏁 Script executed:
# Check if there are any other callback patterns or task management mechanisms
rg "Callback<" -B 2 -A 5 | head -100Repository: dimagi/commcare-android
Length of output: 5346
🏁 Script executed:
# Look at the usage of LatestTaskExecutor in SyncDetailCalculations to understand the impact
rg "unsentFormsExecutor\.submit" -B 5 -A 10Repository: dimagi/commcare-android
Length of output: 1648
🏁 Script executed:
# Check if there are any other mechanisms that might prevent stale results
rg "mainHandler\.post\|Handler\|Looper" app/src/org/commcare/tasks/LatestTaskExecutor.kt -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Verify the actual behavior concern - check if there are concurrent task submission scenarios
rg "submit.*task\|LatestTaskExecutor" app/src/org/commcare/utils/SyncDetailCalculations.java -B 3 -A 15Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check if mainHandler operations have any guards or timeouts
rg "isFinishing\|isDestroyed\|Handler\|post" app/src/org/commcare/utils/SyncDetailCalculations.java -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Read the full SyncDetailCalculations usage
sed -n '1,100p' "app/src/org/commcare/utils/SyncDetailCalculations.java" | head -50Repository: dimagi/commcare-android
Length of output: 1932
🏁 Script executed:
# Check the actual submit call context with broader search
rg "unsentFormsExecutor" -B 5 -A 15Repository: dimagi/commcare-android
Length of output: 2906
🏁 Script executed:
# Check if there are other patterns that show task cancellation race conditions
rg "cancel.*true\|Future\|Callable" app/src/org/commcare/tasks/ -B 2 -A 3Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check for Kotlin formatting baseline - look at nearby Kotlin files for style
fd "\.kt$" app/src/org/commcare/tasks/ -x head -20 {}Repository: dimagi/commcare-android
Length of output: 4540
Prevent canceled tasks from posting stale results.
When a new task is submitted, the previous task's Future is canceled, but Future.cancel(true) is best‑effort—a task that has already computed its result or ignores interruption can still post to the main thread. This creates a race where an older task's callback overwrites a newer task's result. Track a monotonic task ID and check it before invoking the callback to ensure only the latest result is delivered.
🔧 Suggested fix (drop stale callbacks)
import java.util.concurrent.Executors
import java.util.concurrent.Future
+import java.util.concurrent.atomic.AtomicLong
import java.util.concurrent.atomic.AtomicReference
@@
private val singleThreadExecutor: ExecutorService = Executors.newSingleThreadExecutor()
private val currentTask: AtomicReference<Future<*>?> = AtomicReference<Future<*>?>()
private val mainHandler = Handler(Looper.getMainLooper())
+ private val latestTaskId = AtomicLong(0)
fun submit(task: Callable<T>, callback: Callback<T>) {
+ val taskId = latestTaskId.incrementAndGet()
val future = singleThreadExecutor.submit {
try {
val result = task.call()
- mainHandler.post { callback.onResult(result) }
+ mainHandler.post {
+ if (latestTaskId.get() == taskId) {
+ callback.onResult(result)
+ }
+ }
} catch (e: Exception) {
Logger.exception("LatestTaskExecutor task failed", e)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private val singleThreadExecutor: ExecutorService = Executors.newSingleThreadExecutor() | |
| private val currentTask: AtomicReference<Future<*>?> = AtomicReference<Future<*>?>() | |
| private val mainHandler = Handler(Looper.getMainLooper()) | |
| fun submit(task: Callable<T>, callback: Callback<T>) { | |
| val future = singleThreadExecutor.submit { | |
| try { | |
| val result = task.call() | |
| mainHandler.post { callback.onResult(result) } | |
| } catch (e: Exception) { | |
| Logger.exception("LatestTaskExecutor task failed", e) | |
| } | |
| } | |
| val previous = currentTask.getAndSet(future) | |
| previous?.cancel(true) | |
| private val singleThreadExecutor: ExecutorService = Executors.newSingleThreadExecutor() | |
| private val currentTask: AtomicReference<Future<*>?> = AtomicReference<Future<*>?>() | |
| private val mainHandler = Handler(Looper.getMainLooper()) | |
| private val latestTaskId = AtomicLong(0) | |
| fun submit(task: Callable<T>, callback: Callback<T>) { | |
| val taskId = latestTaskId.incrementAndGet() | |
| val future = singleThreadExecutor.submit { | |
| try { | |
| val result = task.call() | |
| mainHandler.post { | |
| if (latestTaskId.get() == taskId) { | |
| callback.onResult(result) | |
| } | |
| } | |
| } catch (e: Exception) { | |
| Logger.exception("LatestTaskExecutor task failed", e) | |
| } | |
| } | |
| val previous = currentTask.getAndSet(future) | |
| previous?.cancel(true) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/tasks/LatestTaskExecutor.kt` around lines 22 - 36, The
submit method can still deliver stale callbacks because Future.cancel(true) is
best‑effort; add a monotonic task id (e.g. an AtomicLong taskCounter) and
associate the id with each submitted work, storing the latest id in a shared
AtomicLong/AtomicReference (or extend currentTask to hold the id). In
LatestTaskExecutor.submit, increment the counter to produce taskId before
submitting, capture that id inside the worker, and before calling
mainHandler.post { callback.onResult(result) } verify the captured id matches
the current/latest id (skip posting if it does not). Keep the existing
cancel(previous) behavior but use the id check to guarantee only the most recent
task's result is delivered.
avazirna
requested review from
Jignesh-dimagi,
OrangeAndGreen,
conroy-ricketts and
shubham1g5
| * Executor that ensures only the latest submitted task is executed, and any previous tasks are cancelled. | ||
| * The result is posted back to the main thread via the provided callback. | ||
| */ | ||
| class LatestTaskExecutor<T> { |
There was a problem hiding this comment.
@avazirna I am curious why did you choose this way instead of going with the more standard Android patterns like Coroutines + Flow/Live Data to handle async processing.
I think we would want to ensure that a solution here -
- is suited for long term and is aligned with Android standards patterns
- Integrates seamlessly with Android lifecycle and doesn't need extra lifecycle management from us (that is the main reason we want to move off from Async tasks as well)
- Ensures that the undelying tasks are cancellable i.e. the DB query should not continue running if the ui component is not alive
- Ensures that Any Failures are not hidden from UI
There was a problem hiding this comment.
I am curious why did you choose this way instead of going with the more standard Android patterns like Coroutines + Flow/Live Data to handle async processing.
I tried to balance the use case with the overhead of invoking coroutines from Java, especially given that CompletableFuture is not an option. I’m happy to revisit this if you have a different perspective.
I agree with all the points you highlighted. Absolutely, coroutines is the recommended approach, however, ExecutorService is still supported
About these two aspects:
Ensures that the undelying tasks are cancellable i.e. the DB query should not continue running if the ui component is not alive
SQLiteDatabase queries are blocking, so even with coroutines, if the ui gets destroyed they will continue running. Right? I can double-check this
Ensures that Any Failures are not hidden from UI
Given the use case, I'm inclined to log exceptions as a non-fatals to avoid impacting the user.
There was a problem hiding this comment.
I tried to balance the use case with the overhead of invoking coroutines from Java, especially given that CompletableFuture is not an option. I’m happy to revisit this if you have a different perspective.
I think we can still use callbacks here with Coroutines, I am imagining it to be like this -
class CoroutineTaskExecutor<T>(
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
) {
fun interface Callback<T> {
fun onResult(result: T)
fun onError(result: Exception)
}
private var currentJob: Job? = null
fun submit(
scope: CoroutineScope,
task: Callable<T>,
callback: Callback<T>,
) {
currentJob?.cancel()
currentJob = scope.launch {
try {
val result = withContext(ioDispatcher) { task.call() }
callback.onResult(result)
} catch (e: Exception) {
callback.onError(e)
}
}
}
}
and then calling this should look similar to -
CoroutineScope scope = LifecycleOwnerKt.getLifecycleScope(activity);
getUnsentFormsExecutor().submit(scope, SyncDetailCalculations::getNumUnsentForms,.. )
Curious do you still think the overheads here are significant to not go this way ?
SQLiteDatabase queries are blocking, so even with coroutines, if the ui gets destroyed they will continue running. Right? I can double-check this
right but Coroutines provide a clear cancellable interface that gets auto-triggered when the related lifecycle component gets cancelled, in this implementation this thread will keep runnning even after the activity is destroyed which is easy to correct but introduces additional state management.
Given the use case, I'm inclined to log exceptions as a non-fatals to avoid impacting the user.
Think that decision depends on the use case and we should not assume that this class will only get used for unsent forms query, the thread should instead expose onSuccess and onError callbacks and let the listeners decide on how to handle the error.
There was a problem hiding this comment.
@shubham1g5 I like this perspective, looks cleaner and it answers all my questions. Let me implement
|
Confirming that everything listed under "QA Plan" has also been tested locally successfully? |
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
I don't have any additional concerns besides the clarifying question I posted previously. Think it'd be good to have another set of eyes on this before merging
| } catch (e: CancellationException) { | ||
| throw e |
There was a problem hiding this comment.
this will crash the app right ? Do we want that ?
| syncStatus += "\n\n"; | ||
| } | ||
| syncStatus += syncIndicator; | ||
| public static LatestTaskExecutor<Integer> getUnsentFormsExecutor() { |
| return; | ||
| } | ||
|
|
||
| Pair<Long, String> lastSyncTimeAndMessage = getLastSyncTimeAndMessage(); |
There was a problem hiding this comment.
can we abstract the UI update logic to another method
|
|
||
| @Override | ||
| public void onError(@NotNull Exception exception) { | ||
| Logger.exception("LatestTaskExecutor task failed", exception); |
There was a problem hiding this comment.
nit: LatestTaskExecutor task failed -> Failed to get unsent forms
3 participants
Product Description
The home screen sync button triggers a query to count unsent forms. This is a database query that is currently executed on the main thread, which can cause ANRs. Over the past 30 days, this ANR has been one of the most frequent, with over 3k occurences.
This PR moves the query to a background thread using a new `LatestTaskExecutor` utility, which posts the result back to the main thread via a callback once the query completes. Rapid successive calls cancel any previous query to avoid stale results.
Technical Summary
Key changes:
LatestTaskExecutor<T>— new Kotlin utility class wrapping a single-thread executor; cancels the previous task on each new submission and posts the result to the main thread via a interfaceCallback<T>SyncDetailCalculations.updateSubText— `getNumUnsentForms()` now runs off the main thread; UI update runs in the callback on the main thread with an activity lifecycle guardSafety Assurance
Safety story
QA Plan
Labels and Review