Fix #4296 + download UI improvement + fixes #4369
Conversation
…wnload UI + missing notify call
| // as this introduces wrong entry in FilesDB due to race condition | ||
| // This is a fix for https://github.com/ente-io/ente/issues/4296 | ||
| await LocalSyncService.instance.getLock().synchronized(() async { | ||
| final File? fileToSave = await getFile(file); |
There was a problem hiding this comment.
We can move this outside. This will fetch the file from remote, and decrypt it.
There was a problem hiding this comment.
I tried moving it outside, and it is technically working (it is not triggering this specific issue), but it starts all selected downloads at same time with following effects:
- app gets very laggy
- likely to have network issues (I have had time outs and network errors because of too many parallel/open requests). Issues which I did not get with the sequential approach.
- it should be faster but in my opinion less robust when dowloading lots of files/complete collection.
At the moment I kept it inside as it is solving the issue while not introducing other ones but I leave it up to you to decide based on this feedback.
A further improvement might be to move it outside and limit the number of concurrent downloads, but maybe not part of this fix/PR?
| // only start if no other asset (or task) requested to stop changes, or if the expiration | ||
| // time for the asset (task) expired. '_processingAssets' should be seen as a queue of | ||
| // open requests. | ||
| class PhotoManagerSafe { |
There was a problem hiding this comment.
Sorry, but to me this feels like additional complexity that we like prefer to avoid adding.
Is it possible to simplify this part either via debouncer or leveraging the fact that all calls to PhotoManager start/stop happens on the same thread?
There was a problem hiding this comment.
I agree, actually it is not even needed with the Local Service Lock (I tested it, but it was already making sense). I have removed it at the moment as it is trying to solve a bigger issue IMO.
…with LocalSyncService lock
Description
Background
Proposal to fix #4296 (@ua741). High level, there is a race condition which is generating wrong entries in the FilesDB (creating orphan files). This issue is easily replicable if you select multiple files and try to download them (the more files, the more likely wrong entries will be created). This issue is also messing up with the date of the files we see in the timeline and is creating "orphan" (not linked to uploaded files) files in the device collections.
To my understanding, this is quite a critical bug and any user is very likely to encounter it in the current implementation.
There is some self healing already in place that will reupload all those files and this should correct the dates, but this will only do it 10 files at a time, and based on potential candidates, without fixing the orphan entries. I was also able to produce files which were never corrected.
This pull request
After extensive tests and lots of debugging, I was able to fully fix this issue (meaning I am not able to reproduce it, at all, and I tried a lot..). I also do not see any orphan in the device collection ("Pictures" folder for Android), while i could see 20+ (even more) each time I was trying to replicate the issue. To fix it, I have:
PhotoManagerSafewhich is safely making sure that the app will not react to file changes when it has been asked not to do so (previous implementation allowed scenarios where a caller was executing code thinking no change notification would be sent).LocalSyncServicethat is used to make sure no local synchronisation can be performed at the same time a download is performed (this was the main culprit). The main side effect is that when you download multiple files, they will be processed sequentially. I think this can be improved but the main "side" advantage is that whenever there is a failure, not a lot will be lost and it is more robust in that way if you want to download a big collection. It would be better to have at least 2 threads downloading/decrypting at the same time, maybe (maybe not needed as well)?Feel free to close this PR if not suitable according to you, I can also take feedback and try to implement them as best as I can.
Tests
Tested on my Pixel 6a
Built with :
Flutter 3.24.3
JDK 17.0.2
Gradle 7.2