Correctly cancels the entity loading task #3017
Conversation
📝 WalkthroughWalkthroughThe changes update the cancellation behavior for entity loading processes. In the Sequence Diagram(s)sequenceDiagram
participant A as EntitySelectActivity
participant L as Loader Task
A->>L: onDestroy() calls loader.cancel(true)
L-->>A: Forcefully terminates ongoing operations
sequenceDiagram
participant Loader as EntityLoaderHelper
participant Ref as References
Loader->>Ref: Begin iterating over references
alt stopLoading is true
Loader-->>Loader: Immediately return current entities list
else
Loader-->>Loader: Process reference and continue loop
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
100-102: Consider consistent return behavior on cancellation.In the new code, cancellation at the beginning of the loop returns partial entities, while cancellation later in the loop still returns
null. This inconsistency might lead to unexpected behavior.Consider making both cancellation paths return the same value:
if (stopLoading) { - return null + return entities }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/activities/EntitySelectActivity.java(1 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/activities/EntitySelectActivity.java (2)
app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
cancel(115-118)app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (1)
cancel(203-206)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
app/src/org/commcare/activities/EntitySelectActivity.java (1)
547-547: Improved task cancellation by allowing interruption.This change sets the
mayInterruptIfRunningparameter totruewhen canceling the loader task during activity destruction. This ensures that the task can be immediately interrupted rather than allowed to complete, which fixes the issue where entity loader tasks continue running even after the activity has been destroyed.app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)
92-94: Added early cancellation check to avoid unnecessary processing.Adding an early exit condition at the beginning of the loop provides a more responsive cancellation mechanism. The task now exits immediately upon detecting cancellation and returns the partially loaded entities rather than continuing to process the remaining references.
This change, together with the
loader.cancel(true)change in EntitySelectActivity, effectively addresses the issue where entity loading tasks continued running after the activity was finished.
…ity as we are not going to reattach to the task again
shubham1g5
force-pushed
the
entityLoadingCancellationBug
branch
from
2677826 to
fcf9a74
Compare
|
I believe this block is no longer needed, given that when |
|
you are absolutely right, thanks for catching - da553e5 |
Product Description
Correctly cancels the entity loading task on finishing activity as we are not going to reattach to the task again.
No user visible change.
Technical Summary
Solves an issue where the entity loader task keeps running even after the entity select activity has been finished due to user getting out of that screen.
This was causing multiple entity loading tasks to get intitiated and run simultaneously when you go back and come in again on the entity list before the entity list has been loaded completely.
The solution here is 2 parts -
Change the mayInterruptIfRunning to
trueto make sure that theonCancelledgets called on time for AsyncTask.Correctly cancels the work by intercepting the
stopLoadingflag which is set totrueon cancellation.Safety Assurance
Safety story
It's a bit confusing to me that we were setting the interrruption flag to
falseon cancellation which is hard to know if it was an oversight or is done due to a particular reason I am unaware of. Although think if we are trying to cancel it's better to interrupt otherwise there is no guarantee on when it well get cancelled.Automated test coverage
None
QA Plan
Not planning
Labels and Review