Make DocumentsProvider operations synchronous

[grote]

This ticket started from #6726 which has more background information and reasoning why this is a good idea. (Seafile does it like this as well.)

I looked into making the entire DocumentsProvider do its operations synchronously on the I/O thread that its methods are already called on. It actually already does this for most operations (move/rename/create/delete) and making the rest synchronous is quite straightforward and would improve the code as well. I'd be happy to provide a PR for that.

The only thing I am not sure about is the use of two SynchronizeFileOperations in DocumentsStorageProvider.

The first one happens when a file to be opened is available locally already. It is probably a good idea to sync this before, when the caller wants to read from the file. If you just want to write into the file, it might not be needed. However, syncing it can mean that async operations like uploads are triggered (and it can reveal conflicts we can't deal with here).
It would be easier if we can assume the local file to up-to-date, but I don't know if there's methods to check for that and how Nextcloud propagates server changes to its local cache.

The second use of SynchronizeFileOperation happens when a caller is done writing to a file and closed it. In this case, we probably don't need to do a sync, but can do a UploadFileRemoteOperation (like in createFile()) right away, right? Or maybe better use a UploadFileOperation so we get chunking for files that are two big.

@tobiasKaminsky do you want me to provide a PR for this and do you have thoughts about making the remaining operations synchronous, especially the two cases above?

@jmue since you contributed most of the to be modified code I would also be interested in your opinion.

[tobiasKaminsky]

Sure, I am happy if you help us in this area 👍

It would be easier if we can assume the local file to up-to-date, but I don't know if there's methods to check for that and how Nextcloud propagates server changes to its local cache.

I fear that this approach might lead to data loss, as server might have changed meanwhile.
If there is a sync conflict, we need to stop writing into it. Or do it like on Desktop client and create a (2) file.

The second use of SynchronizeFileOperation happens when a caller is done writing to a file and closed it. In this case, we probably don't need to do a sync, but can do a UploadFileRemoteOperation (like in createFile()) right away, right? Or maybe better use a UploadFileOperation so we get chunking for files that are two big.

Hm. There I do not remember why I did SFO, but yes, there UFO should be enough.
We only have to make sure that the UFO is writing all updated metadata into our local database.

[grote]

If there is a sync conflict, we need to stop writing into it.

The current code tries to launch a conflict resolver activity from the background which is not allowed anymore in recent Android versions, so we probably need to handle it differently anyway.

When doing SFO here, I would be worried about it triggering other async operations off-thread that might or might not conflict with what we are trying to do.

There's already a check if the local file is downloaded. If it is not, it launches a FileDownloader service which I would replace with a DFO or maybe only ReadFileRemoteOperation like the SFO is using?

So when the file is down already, instead of using SFO, we could directly do a ReadFileRemoteOperation like the SFO would do anyway. However, then we would essentially need to duplicate this code from the SFO which isn't really nice.

Since for the use-case I care about, server changes are not expected, we could also just execute the SFO (on the same thread) when the file is already down. Local changes via the DP should get uploaded right away, so there should be nothing to upload, so no multiple upload requests for the same file (not sure if and how those are handled). If the SFO comes back with a conflict, we could throw a FileNotFoundException (only if mode was writing?), so the operation fails.

Btw. do you know why for the first SFO, you pass in new FileDataStorageManager(user.toPlatformAccount(), context.getContentResolver()) and for the second document.getStorageManager(). Couldn't you use the simpler second one in both cases?

Or do it like on Desktop client and create a (2) file.

I am not a big fan of this, but this is because my consumer of the DocumentsProvider backend is expecting to find things based on filenames and throws if the filename is not as expected. So I am biased here ;)

Hm. There I do not remember why I did SFO, but yes, there UFO should be enough.

Good.

What stroke me as odd is that it passes the same file as local and as server file into the SFO (which might cause some conflict checks not work). But if we replace this with UFO, it doesn't matter anymore.

We only have to make sure that the UFO is writing all updated metadata into our local database.

What metadata is this exactly? UploadsStorageManager#storeUpload()?

Something else to keep in mind is that using UFO directly won't show the Nextcloud upload notifications, but for me this is actually the expected behavior and I wanted to bring this up anyway. Apps interacting with DocumentProviders usually have their own progress bars and notifications. The OS Files app has it and Seedvault also does. I find the flurry of notifications from Nextcloud in addition to the other notification quite annoying, so I wouldn't miss them.

[grote]

From #6726 (comment):

I remember that there was some idea to first display the data stored in db and then fetch the new info in background.
Maybe this is related?

I think not as here we don't return anything, but wait for the Thread to finish.

What you mean is probably the behavior when querying children of a folder. This I could leave as-is as it is somewhat officially intended behavior. However, it can be quite surprising as you first get outdated data and then need to re-query. Even Android experts didn't know about this: CommonsWare about this Nextcloud behavior.

So just let me know what you prefer. For the Android files app that often browses folders that don't change, it might be better behavior, especially since the ReloadFolderDocumentTask is triggered only on time-based cache expiry conditions.

[jmue]

@grote I'm shure that I stumbled on some of questions as you did. Unfortunately I can currently not dive into int as I'm currently on holidays.

[grote]

@tobiasKaminsky I made a first version of the suggested changes in a202b54 Maybe it is easier to discuss the TODOs I left there in this commit, before I do an official PR (lots of bot spam).

@jmue Thanks! Enjoy your holidays! Maybe you get a chance to check out what we ended up doing when you are back.

[tobiasKaminsky]

Maybe it is easier to discuss the TODOs I left there in this commit, before I do an official PR (lots of bot spam).

I answered all of them.
If something is left, we can also have a quick call, which might be easier…

[grote]

For future reference:

The initial goal was to make all operations completely synchronous, so failures can be detected by apps using our DocumentsProvider and there will be no race-conditions by parallel operations.

However, #openDocument() could not be made fully synchronous, because we need to wait for the ParcelFileDescriptor we are returning to close before we can upload a file. Nextcloud works with locally cached files that only get synchronized to
remote storage.

Things I have tried:

• Use the Handler from the calling I/O Thread: not guaranteed to have a prepared Looper, so can't always create Handler on that Thread
• Extend ParcelFileDescriptor and override its close() methods: For some reason they don't get called when the stream gets closed
• notify the content URI when the upload is complete, so callers can wait for the notification: works, but is non-standard. Other DPs are not doing it, so it requires Nextcloud specific code on caller side and is still hacky: what happens if notification doesn't come? Timeout, but for how long?
• use other ways to get a ParcelFileDescriptor on the file: couldn't find anything that would interface with Nextcloud's current architecture