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.

@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?
broken-uploads-list-only-marks-1-uploading-at-a-time
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
broken-notifications

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

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.

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?

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 :)

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.

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

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@alperozturk96 @kra-mo @tobiasKaminsky could I get a re-review

Hello,

thank you for this valuable PR.
Since this is a quite big (and "severe") change, I am a bit of hesitating to merge this soon.
Imagine if in some corner cases (like slow network) none of the parallel uploads finish.

Thus I think we need to have a way to test/make sure that it is robust in many cases.

We have our dev app, which we could use for that.
So I suggest to check if it is possible to bound this to "is_beta" in setup.xml?

This way we can enable it for dev versions, monitor the replies and then at some point enable it.
Before we merge it, we should also do intensive tests on our own.