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: 5693 - able to run tasks without minimum duration wait #5694

Merged
merged 9 commits into from
Oct 20, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

  • There was an obvious specific case about background tasks that wasn't dealt with properly.
  • The goal of the background task manager is to
    • add tasks to a queue
    • execute them one by one
    • restart them later if they failed (e.g. no internet or server down)
  • Very often in the code, we ask the manager to run again, and most of the time nothing is done as there are no tasks in the queue.
  • In order to prevent all the process of managing the queue and all the tasks reordering from happening too often, we say something like "hey, we've just tried less than 5 seconds ago, we'll try later".
  • The thing is there are
    • cases when we don't really care ("oh I've opened a product page, let's see if there are pending tasks, just in case")
    • and cases when we do care ('hey I've just added prices/created the related task, let's run that ASAP")
  • The point of this PR is to be able to say "hey, this time I do care", thanks to a new parameter when running the manager.
  • More specifically about the issue, in the process of adding a task we stored the task and then ran the manager. The problem is that when we store the task we automatically do run the manager before it's ready, and then run it milliseconds later when it says "oh, I'm already running, I'm going to ignore your call". Therefore, now you can say "it's an important call", and have the task run ASAP.

Fixes bug(s)

Impacted files

  • background_task_manager.dart: made it possible to force an immediate run if possible
  • offline_tasks_page.dart: force an immediate run if possible when clicking on the refresh button

Impacted files:
* `background_task_manager.dart`: made it possible to force an immediate run if possible
* `offline_tasks_page.dart`: force an immediate run if possible when clicking on the refresh button
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 6.95%. Comparing base (4d9c7fc) to head (fe59c9d).
Report is 377 commits behind head on develop.

Files with missing lines Patch % Lines
...th_app/lib/background/background_task_manager.dart 78.57% 3 Missing ⚠️
...ckages/smooth_app/lib/database/local_database.dart 0.00% 3 Missing ⚠️
...kages/smooth_app/lib/pages/offline_tasks_page.dart 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5694      +/-   ##
==========================================
- Coverage     9.54%   6.95%   -2.60%     
==========================================
  Files          325     404      +79     
  Lines        16411   21589    +5178     
==========================================
- Hits          1567    1502      -65     
- Misses       14844   20087    +5243     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teolemon teolemon requested a review from g123k October 13, 2024 22:06
Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

The description sounds enticing, and if we really did that, then that could explain some of the PlayStore complaints. We should really aim for instant results, and honesty in our messaging (perhaps a "changes submitted indicator" ?)

@monsieurtanuki monsieurtanuki merged commit a8cfcd0 into openfoodfacts:develop Oct 20, 2024
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon for your review!

The description sounds enticing

thanks

and if we really did that, then that could explain some of the PlayStore complaints.

I don't think so, unless the complaints were targeted at Prices and during a specific month when we refreshed the background tasks "too early".
Regarding "normal" tasks like product edits, there were tons of places when we refresh the background tasks (virtually every product page), and we display the local changes immediately.
cf. #5405, where the best solutions would be:

  • to report immediately and verbosely any issue when we see something wrong in background tasks, in order to be more precise about what goes wrong
  • to split the background tasks (unique queue currently: potentially split by long image uploads vs. short tasks, and split by servers)

We should really aim for instant results, and honesty in our messaging (perhaps a "changes submitted indicator" ?)

Probably worth a specific issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Prices are not uploaded immediately
3 participants