Background + offline incoherences on multiple offline edits #3018

monsieurtanuki · 2022-09-15T15:22:02Z

What

Assuming that I use the app offline and that I change a product 5 times in a row (changes A, B, C, D and E), there are 5 background processes pending, all on the same product.

When I go online, I may have problems.

Problem 1: if each process is executed atomically in a queue, after I run processes about A and B, I'll get the server product with changes A and B, but not yet C, D and E. The product is less complete than when I was offline with all my changes.
- Solution 1: merge all changes about the same product into a single background process before executing it.
Problem 2: if I try to refresh the product before the background processes are executed, I'll get a fresh product from the server but without my local changes.
- Solution 2: we should not be able to refresh a product when there are pending background processes. In addition to that, we could add an icon "local changes pending" and an error message "cannot refresh product while there are local changes pending".
Problem 3, related to problem 1 (spoiler alert: bad luck problem): if while the merged background processes are executed I change the product (change "F"), after the execution I get a fresh server product with all my initial changes (A-E), but change F will be ignored, until background process about F is executed.
- Solution 3: each time we get product data from the server, we have to apply the pending changes before storing it locally.

The more I think about it, the more I believe that solution 3 would solve everything (we wouldn't need solutions 1 and 2).

@AshAman999 Do you share my views?

anshpathania7 · 2022-09-15T15:42:42Z

This sounds similar to performing commits on git.

what if we handle this like priority queue?

All local changes are prioritized over server retrieval.
if new change is submitted during execution of local changes, it will be added for execution before server retrieval.

if server retrieval is in execution, local changes will be executed afterwards and server will be re-fetched? (not sure about this)

monsieurtanuki · 2022-09-15T16:11:24Z

This sounds similar to performing commits on git.

Maybe, I don't understand that much about git - and I don't intend to.

what if we handle this like priority queue?
All local changes are prioritized over server retrieval.

Sounds correct, except that we need a decent UX for that - like the "cannot refresh product while there are local changes pending" message I was mentioning before.

if new change is submitted during execution of local changes, it will be added for execution before server retrieval.

How do you do that? For the moment, the background process execution is first the "upload changes" (async), and then the "download product" (which is async too).
And in the meanwhile - after the first execution and before the second one - how is the product?

if server retrieval is in execution, local changes will be executed afterwards and server will be re-fetched? (not sure about this)

That's more or less my "solution 3".

AshAman999 · 2022-09-15T16:27:15Z

Problem 1: if each process is executed atomically in a queue, after I run processes about A and B, I'll get the server product with changes A and B, but not yet C, D and E. The product is less complete than when I was offline with all my changes.

Solution 1: merge all changes about the same product into a single background process before executing it.

@monsieurtanuki
Thanks for raising the concern, what you propose looks good to me

There was something similar like sol 1 in my mind to do

I believe that a mix of sol 1 and sol 3 would work the best,

we check for the list of pending background processes, we merge the changes for a product, (would be better to have less API calls), and show an error snack bar too, if background task fails.
Something similar to what google keep having, show a sync icon in the product card, if the background sync process is in process of change, would be visually confirmation for the same
For product card, we should use a consumer i.e. provider so that the UI is rebuilt fully in case of changes in the product

anshpathania7 · 2022-09-15T16:31:55Z

How do you do that? For the moment, the background process execution is first the "upload changes" (async), and then the "download product" (which is async too).
And in the meanwhile - after the first execution and before the second one - how is the product?

upload changes => download product

when new local change is added, it's pushed to execute before 'download product'

upload changes => new_local_changes => download product

if download is in progress, we can either stop it and perform upload of new_local_changes first

or we can wait for download to complete then perform the flow of upload and download again?

monsieurtanuki · 2022-09-15T16:56:22Z

We need to update the local changes instantly. Then queue a background process that uploads, then downloads, then stores locally.
Therefore I'm not sure what you suggest (upload then local changes) would work.

if download is in progress, we can either stop it

I don't think we can stop an async, can we?

Sharing views on that issue, I think we should just ignore whatever user data refresh (pull-to-refresh, search) from the server while there are pending tasks in general.
A possible nice-to-have would be to do that at the product level (user can't refresh a product while there are pending processes on that product), but that may be confusing for the user (if we refresh a whole search, most products will be fully refreshed while some "pending" products should not be).

monsieurtanuki · 2022-09-15T16:57:41Z

I just want a solution that works - possibly bullet proof. Solution 1 so far looks like a "nice to have" and won't solve the main issue. Which means we can forget it. When all the major bugs are solved, then we can think about it again.

We won't show an error snack bar from a background process. Or with difficulties.

I don't think we need a provider, we just need a notifier. Like LocalDatabase.

I don't know google keep.

anshpathania7 · 2022-09-15T17:09:57Z

Therefore I'm not sure what you suggest (upload then local changes) would work

If somehow upload fails, then we'll need to revert back local changes?

I don't think we can stop an async, can we?

yeah, this can be used CancelableOperation

Sharing views on that issue, I think we should just ignore whatever user data refresh (pull-to-refresh, search) from the server while there are pending tasks in general.

Yeah, can add an indicator on top to let user know that they can't refresh due to background tasks

monsieurtanuki · 2022-09-15T17:31:33Z

If somehow upload fails, then we'll need to revert back local changes?

That's a good point: I have no idea what we are supposed to do if upload fails.

yeah, this can be used CancelableOperation

Yes and no: the server will still process the data, it's just that the returned value on flutter will be irrelevant, right?

Yeah, can add an indicator on top to let user know that they can't refresh due to background tasks

Not that easy, as the retry process of background tasks sometimes waits 3 hours before launching again.

monsieurtanuki · 2022-09-15T17:46:10Z

@teolemon @AshAman999 @anshpathania7 So if I had to sum up today's brainstorming, we are currently stuck with this question:

<< What are we supposed to do when a background "update product" process fails? >>

retry 5 times, first after 30 seconds, then after 5 mins, then 30 mins, then an hour, then 3 (current implementation)
=> that only delays the answer to the point when it failed 5 times (possibly because the server is down or the internet connection is poor) (and if the connection is poor it may mean that the server did process the change but was no able to return a success code)
try forever (how often?)
roll back locally the related changes (how do we warn the user?)

anshpathania7 · 2022-09-15T17:48:17Z

@monsieurtanuki

Yes and no: the server will still process the data, it's just that the returned value on flutter will be irrelevant, right?

Yes, it will only cancel async tasks on flutter side, irrespective of Api.
Instead of cancelling we can just let it execute, also are we storing state of these operations every second?
So in case of forced shut down of application we can refer the state saved locally?

This might also be helpful in knowing which tasks were failed so we can revert local changes for it

Not that easy, as the retry process of background tasks

if 5 retry methods fail,
how about adding a periodic upload at some fixed time daily (like 12 am)? To clear up pending tasks?

Or

we can stack up all tasks to execute in slots, like having 4 slots of 6 hours interval,
And at each interval, we execute the current queue.

post execution queue is cleared, so it can be used in next time slot

monsieurtanuki · 2022-09-15T18:07:26Z

We cannot use slots: we want the data to be uploaded to the server as soon as internet is available.
So that beyond the actual user changes, everyone (including the user) benefits from the impact of the changes (e.g. "ecoscore" computed on the server side after the user added a category to a product and uploaded the change) (and then downloaded its change + the new ecoscore).

anshpathania7 · 2022-09-15T18:13:14Z

what if slots are only used for failed upload tasks? (which were failed even after 5 retries)

monsieurtanuki · 2022-09-16T06:56:29Z

In those cases I always ask myself: how do other apps do?

It looks like Evernote does it that way:

automatic upload of the note if online
explicit "not uploaded yet" icon
"sync" button
manual conflict management in some helpless cases

Which inspires me several things:

whatever we should do should be on the product level (for instance not a queue for all individual changes, but a queue of changed products)
we should make explicit that this product still needs to be sync'ed, and show a "sync/upload" button
every time we refresh a product (e.g. pull to refresh) we should upload it first (if relevant)

anshpathania7 · 2022-09-16T11:40:40Z

whatever we should do should be on the product level (for instance not a queue for all individual changes, but a queue of changed products)

Sounds good to me, so in queue of changed products, each product will have all changes to that specific product?

(Like queue of Product Object, where Object holds all changes to that product) ?

we should make explicit that this product still needs to be sync'ed, and show a "sync/upload" button

Yes, adding this globally would be better, like showing it on top of every screen

every time we refresh a product (e.g. pull to refresh) we should upload it first (if relevant)

Or we can explicitly mention to sync local changes first then refresh

monsieurtanuki · 2022-09-16T17:37:24Z

@anshpathania7 I'm not sure exactly if we can do that currently, so let's check first!

@AshAman999 @g123k If I've understood well, for the moment there's a queue of background tasks, handled by the task manager. That sounds cool, we just add tasks and let the manager manage.
Unfortunately it looks like we need to be a bit intrusive with the task manager data, as we need to know from outside of the manager:

the list of related barcodes pending
the actual action of each task
with the option of merging all the tasks related to a barcode into a single task

Is that doable with the current task manager?

monsieurtanuki · 2022-09-17T14:47:40Z

I think I've found a solution in order to merge all the tasks related to a product.

Today, when we add a task we say something like "for barcode 0123, change category to XXX".
Which means there's a new entry in the task queue, with a key like 0123_CATEGORY and a value like category: XXX.
If later the category is changed again, it will use the same task key 0123_CATEGORY. Or something like that.

What if instead of adding this task in the task manager, we created a dedicated sqflite table that would contain the same changes, and we just add to the task manager a simple task like 0123_CHANGE (no other parameter).
How would the task manager execute task 0123_CHANGE? We list all the changes recorded in our new table, merge them, execute them as one, and clean the new table about barcode 0123.
Et voilà.

AshAman999 · 2022-09-17T17:44:41Z

Hello @monsieurtanuki,

The solution you are suggesting does looks promising,
About other things here, like what could be easily done, with the task_manager package
Ahh about the things related to the package task_manager,

We can request details about what was given to the Task object when we scheduled the task,
So at any instant, if a task is not running I can get a list of those tasks from the task manager,
Also, we can cancel those tasks with their unique id,
What we can do is,

Before queuing a task, look if, in pending tasks, there is a key that contains the same barcode as this one
if it is we gather the object from there,
We extract the product from that Taskobject
cancel that task,
merge the change we want now with the last one,
and then ask the task_manager to perform the new task

I am not sure about which way to go, perhaps I can work on the one I am suggesting, and see if works,
Though your approach seems faster and more efficient ...

monsieurtanuki · 2022-09-17T18:45:45Z

@AshAman999 I guess your algorithm makes sense.

That said, the added value of the task manager is to queue tasks and run them when the internet connection is OK. It's supposed to be a black box that does that very well. As we have functional needs that go beyond the standard use of the task manager, we should keep the functional features in smoothie (including the task merge) and the task queuing in task manager. Of course we could do everything with task manager if our needs were simpler, like "for a barcode, execute only the latest task". To be discussed.

Anyway, it's not the priority now: I just mentioned our requirements to double-check that in the end, we'll have something that works in all cases.

The priority now is to get rid of Consumer<UpToDateProductProvider>, that prevents us from notifying the app. The only low level notifier we have is LocalDatabase, and that's where (or in DaoProduct) we should move UpToDateProductProvider's code to.
With that, we'll be able to notify the app every time a background task is finished, which we cannot do now.
Actually I've spent one hour on it so far. If you want to do it, tell me and I'll scrap my one-hour changes.

AshAman999 · 2022-09-18T06:50:29Z

Actually I've spent one hour on it so far. If you want to do it, tell me and I'll scrap my one-hour changes.

I have done none for now, first I guess if we could refresh views after the background task is done, would be a good start

Then we could do about the consideration of merging changes into one.

monsieurtanuki · 2022-09-18T09:23:24Z

@AshAman999 OK, I go on with my changes then. I'll keep you posted.

anshpathania7 · 2022-09-18T18:09:56Z

The priority now is to get rid of Consumer, that prevents us from notifying the app

@monsieurtanuki How is this preventing notifying the app? Like what's the expected behaviour?

monsieurtanuki · 2022-11-05T11:31:06Z

Related issues:

P0:

P1:

P2:

(Product Addition) A button to open nutrition extraction should appear after upload of the nutrition photo (and not immediately like currently) #2978
Image Gallery - The gallery is not refreshed on new image upload #2961

P3:

(Product Addition) Buttons to extract/input data from images should appear after upload of the relevant photo #1398

Other

New file: * `dao_transient_operation.dart`: Transient operation: a minimalist product with a key. Impacted files: * `barcode_product_query.dart`: minor refactoring * `local_database.dart`: added reference to new class `DaoTransientOperation` * `Podfile.lock`: wtf * `product_refresher.dart`: minor refactoring * `pubspec.lock`: wtf

monsieurtanuki · 2022-11-11T12:48:14Z

Thank you @M123-dev!
Aren't we supposed to say "Closes: #xxx" or "Fixes: #xxx"?
It looks like if in the PR comment we just mention an issue, it is going to be closed.

M123-dev · 2022-11-11T12:55:45Z

Yeah you are right, don't know why it closes, maybe because of your commit name

monsieurtanuki · 2022-11-11T20:47:53Z

@M123-dev It looks like you're right: in #3308's title I didn't put the hash sign for issue 3018, which was not closed after the PR was merged.

monsieurtanuki · 2022-11-19T13:47:24Z

Closed by #3329. Now the focus is on #3263.

monsieurtanuki added the 🐛 bug Something isn't working label Sep 15, 2022

teolemon added this to 🤳🥫 The Open Food Facts mobile app (Android & iOS) Sep 15, 2022

teolemon moved this to To discuss and validate in 🤳🥫 The Open Food Facts mobile app (Android & iOS) Sep 15, 2022

monsieurtanuki mentioned this issue Sep 15, 2022

Something is going wrong with nutrition edit #3006

Closed

monsieurtanuki mentioned this issue Sep 16, 2022

refactor: backgroundTasks - around the new "upload" method #3028

Merged

teolemon changed the title ~~Background + offline incoherences~~ Background + offline incoherences on multiple offline edits Sep 18, 2022

teolemon added Offline refactoring Offline - Contribution labels Sep 18, 2022

monsieurtanuki mentioned this issue Nov 4, 2022

Background tasks and pending operations in Smoothie #3263

Closed

M123-dev added this to the v4 milestone Nov 6, 2022

monsieurtanuki mentioned this issue Nov 10, 2022

fix: #3018 - storing the pending changes #3302

Merged

monsieurtanuki closed this as completed in 316272e Nov 10, 2022

github-actions bot mentioned this issue Nov 10, 2022

chore(develop): release 4.1.0 #3305

Merged

M123-dev reopened this Nov 11, 2022

monsieurtanuki mentioned this issue Nov 11, 2022

fix: 3018 - immediate local and server refresh for details (temporary) #3308

Merged

monsieurtanuki closed this as completed Nov 19, 2022

monsieurtanuki mentioned this issue Dec 24, 2022

Check needed for return status of server changes #3466

Closed

monsieurtanuki mentioned this issue Jan 6, 2023

fix: 3533 - runs each task only once #3541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background + offline incoherences on multiple offline edits #3018

Background + offline incoherences on multiple offline edits #3018

monsieurtanuki commented Sep 15, 2022 •

edited by teolemon

Loading

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

AshAman999 commented Sep 15, 2022 •

edited

Loading

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 16, 2022

anshpathania7 commented Sep 16, 2022

monsieurtanuki commented Sep 16, 2022

monsieurtanuki commented Sep 17, 2022

AshAman999 commented Sep 17, 2022

monsieurtanuki commented Sep 17, 2022

AshAman999 commented Sep 18, 2022

monsieurtanuki commented Sep 18, 2022

anshpathania7 commented Sep 18, 2022

monsieurtanuki commented Nov 5, 2022 •

edited

Loading

monsieurtanuki commented Nov 11, 2022

M123-dev commented Nov 11, 2022

monsieurtanuki commented Nov 11, 2022

monsieurtanuki commented Nov 19, 2022

Background + offline incoherences on multiple offline edits #3018

Background + offline incoherences on multiple offline edits #3018

Comments

monsieurtanuki commented Sep 15, 2022 • edited by teolemon Loading

What

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

AshAman999 commented Sep 15, 2022 • edited Loading

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 15, 2022

anshpathania7 commented Sep 15, 2022

monsieurtanuki commented Sep 16, 2022

anshpathania7 commented Sep 16, 2022

monsieurtanuki commented Sep 16, 2022

monsieurtanuki commented Sep 17, 2022

AshAman999 commented Sep 17, 2022

monsieurtanuki commented Sep 17, 2022

AshAman999 commented Sep 18, 2022

monsieurtanuki commented Sep 18, 2022

anshpathania7 commented Sep 18, 2022

monsieurtanuki commented Nov 5, 2022 • edited Loading

monsieurtanuki commented Nov 11, 2022

M123-dev commented Nov 11, 2022

monsieurtanuki commented Nov 11, 2022

monsieurtanuki commented Nov 19, 2022

monsieurtanuki commented Sep 15, 2022 •

edited by teolemon

Loading

AshAman999 commented Sep 15, 2022 •

edited

Loading

monsieurtanuki commented Nov 5, 2022 •

edited

Loading