Parallelise uploads

[k0raph]

Issue: #10124

This PR aims to introduce improvements to the file upload process by parallelizing uploads using Kotlin Coroutines and a Semaphore (which I defaulted to 10 concurrent tasks) to improve throughput.

Key Changes

1. Changing FileUploadWorker.kt to support parallel Uploads
2. Compatibility change in FileUploadHelper.kt
3. Unit tests for FileUploadWorker.kt (extracted out dependencies to make testing easier)
4. IT for (batch) uploading of files
5. Shared preference "max_concurrent_uploads" to allow users to configure their own concurrency limits

• Tests written, or not not needed

[alperozturk96]

Hello

Thank you for the PR 👍

I really appreciate the time and effort you’ve put into this.

I wanted to share some feedback after reviewing the changes. Using ConcurrentHashMap, Semaphore, atomics, and synchronized blocks to achieve parallel uploads makes the implementation quite fragile and hard to maintain in the long run. It also increases complexity in an area of the codebase that is already fairly sensitive.

In fact, we can achieve parallel uploads with a much simpler change. Currently, in BackgroundJobManagerImpl, the startFilesUploadJob function uses the same worker tag together with the ExistingWorkPolicy.APPEND_OR_REPLACE policy. Because of this, a new worker is not used for each upload attempt; instead, uploads are queued and executed sequentially.

By making the tag unique (for example, by appending a random value), a new worker will be started for each upload, enabling parallelism with minimal changes:

val tag = startFileUploadJobTag(user.accountName + Random.nextInt())

---

Introducing parallel uploads is definitely a good idea, but it’s something we should approach incrementally and carefully to ensure the overall robustness of the app. FileUploadWorker is a critical component: it also triggers UI updates via the broadcast manager and is observed by multiple screens. Making it parallel will likely have side effects in those areas (notifications, upload list, file listing, etc.), so we need to be mindful of that impact.

I’d suggest the following approach:

1. Enable parallel uploads with the minimal change (unique worker tags).

2. Observe and adjust the behavior of the caller functions, and add appropriate tests.

3. Observe UI behavior (notifications, file list, upload list) and address any issues that arise.

Thanks again for your contribution and initiative on this topic. :)

@ZetaTom @tobiasKaminsky fyi.

[k0raph]

Hello

Thank you for the PR 👍

I really appreciate the time and effort you’ve put into this.

I wanted to share some feedback after reviewing the changes. Using ConcurrentHashMap, Semaphore, atomics, and synchronized blocks to achieve parallel uploads makes the implementation quite fragile and hard to maintain in the long run. It also increases complexity in an area of the codebase that is already fairly sensitive.

In fact, we can achieve parallel uploads with a much simpler change. Currently, in BackgroundJobManagerImpl, the startFilesUploadJob function uses the same worker tag together with the ExistingWorkPolicy.APPEND_OR_REPLACE policy. Because of this, a new worker is not used for each upload attempt; instead, uploads are queued and executed sequentially.

By making the tag unique (for example, by appending a random value), a new worker will be started for each upload, enabling parallelism with minimal changes:

val tag = startFileUploadJobTag(user.accountName + Random.nextInt())

Introducing parallel uploads is definitely a good idea, but it’s something we should approach incrementally and carefully to ensure the overall robustness of the app. FileUploadWorker is a critical component: it also triggers UI updates via the broadcast manager and is observed by multiple screens. Making it parallel will likely have side effects in those areas (notifications, upload list, file listing, etc.), so we need to be mindful of that impact.

I’d suggest the following approach:

1. Enable parallel uploads with the minimal change (unique worker tags).

2. Observe and adjust the behavior of the caller functions, and add appropriate tests.

3. Observe UI behavior (notifications, file list, upload list) and address any issues that arise.

Thanks again for your contribution and initiative on this topic. :)

@ZetaTom @tobiasKaminsky fyi.

Thank you @alperozturk96 for taking the time to review and I appreciate you sharing your expertise in this area. I don't disagree with any of the comments and I will happily explore your suggestion.

[k0raph]

@alperozturk96 I have looked into your suggestion, and managed to get it working. It's significantly less changes and anecdotally seems to run much faster =]. Unfortunately it has had the side effects you mentioned. Notably the upload list is not reflective of the parallel nature (multiple files should be marked as uploading) - I am not sure how to address this without the ConcurrentHashMap solution I previously incorporated into FileUploadWorker. Could you please advise in this case?

Additionally the notifications are now also broken, the numerators are all over the place - I don't have a solution for this yet. Will investigate

May I also clarify what you mean by "file list"?

[kra-mo]

From a design POV, "max concurrent uploads" should definitely not be a user-configurable setting. Just pick a sensible default and stick with that. If it doesn't work for everyone, we should investigate then.

[k0raph]

From a design POV, "max concurrent uploads" should definitely not be a user-configurable setting. Just pick a sensible default and stick with that. If it doesn't work for everyone, we should investigate then.

Hey @kra-mo happy to remove the configurability and simply default, but could you help me understand why it "should definitely not be user configurable?". Seems like a useful reasonable option at a glance

Alternatively if the fear is that we are giving the user "a foot gun", we can provide a limited selection of options 1,2,5,10,20?

[kra-mo]

It's against our design principles:

Software should get out of the way. Do things automatically instead of offering configuration options. When people ask for a setting, find out what the root of the problem is and fix that instead. Also read Choosing our Preferences.

From https://docs.nextcloud.com/server/27/developer_manual/design/introduction.html

But TL;DR the more preferences you have that most users shouldn't touch, the harder it is to find ones that they do care about. If there is no reason for something to be configurable other than "why not", it's best to just not :)

[kra-mo]

If there is a need to have different numbers of for different scenarios, it'll probably come up, and we can probably solve it for that scenario automatically, as the developers. We shouldn't put the burden on the user.

[k0raph]

@kra-mo great response, consider me convinced. I will revert the changes and default