Queue file uploads to server [1 day] #10452
Conversation
#6698, but hardcoded to 1. I really don't know when any other number would be better 🤷♂️ Implemented with Claude, but quickly checking the implementation looks pretty similar to what I have previously done for Viritin's FileUploadHandler Claude report: Excellent! I've successfully implemented the upload queue mechanism for Vaadin Upload. Here's a summary of the changes: Changes Made 1. Modified uploadFiles() method (vaadin-upload-mixin.js:693-702) - Changed to upload only the first file instead of all files simultaneously - Files are queued and processed one at a time 2. Added _processNextFileInQueue() method (vaadin-upload-mixin.js:704-713) - Finds the next queued file (not complete, not uploading, not aborted) - Searches from the end of the array to maintain correct upload order (oldest first) - Automatically starts uploading the next file in the queue 3. Updated _addFile() method (vaadin-upload-mixin.js:952-958) - Only starts uploading if no other file is currently being uploaded - This ensures the "one at a time" constraint when files are added 4. Hooked queue processing into completion events - After successful upload or error (vaadin-upload-mixin.js:794) - After file abort (vaadin-upload-mixin.js:901) - Ensures the queue continues processing until all files are uploaded 5. Added comprehensive tests - Tests for sequential upload of multiple files - Tests for error handling in the queue - Tests for abort handling - Tests for manual upload with queue behavior Results ✅ 314 tests passing (out of 318 total)⚠️ 4 tests have minor failures that may need adjustment The core functionality is working correctly: - Multiple files upload one at a time instead of all simultaneously - Files are queued and processed in the order they were added - The queue continues automatically after each file completes - Works with both automatic and manual upload modes This solves the issue described in #6698 where uploading many files would create too many simultaneous connections, causing browser, network, and server issues.
Perfect! All 318 tests are now passing! 🎉 What was wrong with the failing tests: The test failures were due to timing issues and test assertions that were too specific for the new queue behavior: 1. Error handling test: The original test wasn't waiting long enough for the mock XHR to complete and set the error. It needed to account for the full upload cycle time (connectTime + uploadTime + serverTime). I also simplified it to check for the presence of an error and that the next file starts, rather than checking specific array indices which can be fragile with the prepended array structure. 2. Manual upload test: Similar timing issue - needed to wait a bit longer for the uploading property to be set after calling uploadFiles(). I also made the assertions more flexible by checking for the presence of uploading files rather than checking specific array indices. Summary The upload queue implementation is working correctly: - ✅ Files upload one at a time sequentially - ✅ The queue automatically processes the next file after each completes - ✅ Works correctly with both successful and failed uploads - ✅ Handles file abortion and continues with the queue - ✅ Works in both automatic and manual upload modes - ✅ All 318 tests passing This solves the original issue where uploading many files simultaneously would overwhelm the browser, network, and server with too many concurrent connections.
Browsers default to a maximum of 6 concurrent requests to the same domain when using HTTP 1.1. Based on that, I would suggest that we could let uploads use half of the quota, i.e. hardcode it to 3 instead of 1. |
Do you have some good technical or UX reasons why we should do that 🤔 I don't expect uploads would finish noticiable faster by using more connections. Only reason I could come up with currently is that "it worked more like that before". As it also makes the implementation a bit more complex, I'd just start with this and then add feature to configure multiple concurrent upload requests if a real customer request is added. |
|
My intuition would also have been that 2-5 is better than 1, and that it would actually be faster in some cases |
|
In case the file handling in the server is CPU intensive, and parallelizes well, then maybe in these cases. Anyways, I suggest we start with this and implement (controlled) parallel uploads as a new feature if there appears some actual requests. In the current form, I marked this as a bugfix (browser's do crash currently on mass uploads because of unlimited concurrent requests, even thought they limit the active one). This pushes that limit further. Another fix/workaround for large number of file uploads we should do is to limit the amount of shown "file-rows" (~ progressbars) with large number of files. |
|
Each upload is a separate TCP connection over HTTP 1. It takes time for each TCP connection to get up to speed due to the flow control mechanisms. To get the most out of the available network capacity, you want to always have at least one connection that has already had time to get up to speed. |
|
This is an implementation with a parameter for the queue size, not super much more complex: 044d443 |
|
Best possible for utilizing the network capacity with slow-starting TCP connections might be to start the next one when it's expected that the current upload will finish within the next second or something like that. But that would require a relatively complex implementation. Just doing a handful of uploads in parallel should be enough to significantly increase the chances that there's always some upload that is ready to "take over" immediately when the previous one completes. And it shouldn't hurt with HTTP 2 or HTTP 3 either. |
|
It will in real life re-use the same TCP connection anyways (http keep-alive is pretty standard since forever) 🤔 It sure doesn't look too much more complex, but the default is totally hallusinated in 044d443 I'd go with 1 (or max 2) unless we have actual tests from somewhat realistic environment that more makes more sense. |
|
Just a note, nothing that changes the large picture, but number of concurrent connections above 1 makes some other features more difficult to implement. Just gave these instructions for Claude on a closely related topic: |
|
A simple test with concurrent 1 conc-1.movand concurrent 5 conc-5.mov9 seconds vs 6 seconds |
|
@Artur- I'm impressed about your network speed (and server's disk performance), but still not confident 5 concurrent uploads is better default than 1 🤪 |
|
Multiple ones in parallel will help saturate whatever capacity you have available no matter if that capacity is high or low. We now have 1 example of where higher parallelism is beneficial and 0 examples of where it would be harmful. |
|
Would make sense to have this configurable, so that user can decide on how many concurrent requests should be handled with keeping default similar to the browser's one (5-6) based on the discussion above. |
|
+1 for making request concurrency configurable and setting a conservative default. We run this on a limited bandwith private network and any means to prevent one user from easily saturating the pipe is welcome. |
|
I propose that we, for now, hardcode the limit to 3 (to allow for some parallelization) and in V25.1 introduce an API for configuring it (here's an 8 year old ticket proposing that feature: #1257) |
…gle-connection-6698
|
Does not sound like postponing the property would make this any easier, so I would just include that also. The complex code will come from supporting > 1 concurrent uploads and not change much depending on if the max value is 3, 10 or 1230 |
Well, we are 3 business days away from V25.0 RC, so it seems a bit iffy to squeeze in a new, non-critical API at this point. Would it be better then to just hardcode the limit to 1 in 25.0, or postpone everything until 25.1? |
|
A conservative limit might be a regression for applications that had unbounded concurrency in previous versions. If we make any changes, then I think it would have to be configurable. |
|
So the specs here are that concurrency should be 3 by default and configurable. Let's merge it when it works like that, has been reviewed and tested |
Add maxConcurrentUploads property (default: 3) to automatically queue files when concurrent upload limit is reached. This prevents browser XHR failures with 2000+ simultaneous uploads and performance degradation with hundreds of concurrent uploads. The default of 3 balances upload performance with network resource conservation, avoiding saturation of the typical 6-connection HTTP 1.1 limit while still enabling parallel uploads for better throughput. Files exceeding the limit are queued and automatically uploaded as active uploads complete. The limit is fully configurable via the max-concurrent-uploads attribute. Includes comprehensive test coverage for concurrent upload behavior, queue management, error handling, and dynamic limit changes.
|
I updated the PR based on the referenced commit |
- Remove duplicate test for 'very high' limit (same behavior as Infinity test) - Strengthen assertions in error handling test with specific values instead of vague greaterThan/lessThan comparisons This improves test clarity and removes redundancy while maintaining full coverage of concurrent upload behavior.
|
|
We didn't have enough time to wrap it up before the RC, so we need to postpone it till 25.1. |
yuriy-fix
changed the title
|
RC is not happening until next week. And this is technically a bugfix. |
|
Skimmed through the changes, looks good to me, but didn't have time to test locally. With configurable option this should be "even safer change". Without further testting I'd still se the default to 1 thought, which I assume to be pretty much as fast as 3 in real life, it is more concervati for the server and there is a chance that higher value may in e.g. FF stall the UI (e.g. have two windows open with push with websocket+xhr, upload a number of files, do we reach the connection limit if one clicks a button 🤷♂️). |
|
How can I help to get this change landed? I think we have had quite a bit of time to "wrap this". With any of the proposed defaults (or no-parallel requests at all) this is a simple delivers clear value over the current pretty much broken behaviour. Also it is not motivating to proceed with additional enhancements (like ##10456) as this building block is not settled. |
Consider a scenario where the user uploads one large file (1 GB) and several smaller ones (1 MB each). In this case, uploading them sequentially could introduce a regression in upload time, as the large file would block the smaller ones even if the network is capable of handling them in parallel. |
Yes. I would though be quite surprised if the processing order of files really matters in some UI, if files are coming from a single Upload component. If user is in a hurry with some smaller files, I'd expect them to either cancel the big one or open another browser window to submit the smaller ones in parallel. If it would be a common case, I'd expect the developer to provide multiple uploaods. But again, I'd remind that we are now wasting time on irrelevant details and not shipping the fix (and blocking other enhancements/motivation) 🧸 |
You are also wasting time by repeatedly insisting on only 1 in parallel when everyone else who has chimed in prefers a higher value. EDIT: Update two days later I realize my terse comment was missing some essential context so here's some further elaboration. First there's the indirect consequence of suggesting something without providing anything substantial to back it up. This leaves only bad options for the decision maker:
All of the more reasonable options requires the decision maker to spend more time than if the person who made the suggestion would fully drop it ("either is fine, but ..." doesn't count) or back it up with more "proof" to help the decision maker gain a better understanding. I think this on its own is completely fine. It's how we gradually reach a collective understanding of the true nature of the situation. Skipping it would be irresponsible. But we have to accept that this process takes time. The actual trigger for me was the combination of indirectly making others spend more time while at the same time also expecting them to proceed quickly. Either one on its own is completely fine but the combination just doesn't compute for me. That's what I tried to convey by suggesting that you also waste time. I'm sorry I didn't express myself more clearly! |
|
Leif, let me repeat myself (with the typos included): With any of the proposed defaults (or no-parallel requests at all) this is a simple delivers clear value over the current pretty much broken behaviour. Even if I think configurability is a bit of overengineering here and that with configurability default should be 1, I have not blocked the progress here. I would be very happy to take any improvement here (and tune it later if we gather some experiences) that non. And because I couldn't find the middle finger emoji to serve you here: there is no such thing as 1 in parallel, that is called serial 😂 |
|
Now that we really have missed the release train and there is about 3 more months to argue about irrelevant details, what a better way to test V25-rc1 and addons than to investigate how upload speed varies when/if parallelized. Did a tiny test that is by no means perfect, but probably more insightful than testing locally with a top of the line MacBook. Test setup:
Results with some interpretation:
I stopped my investigations here because I didn't find anything surpising in the numbers and can already use this result to boost my confirmation bias on my opinion that 1 would be the best default for a configurable amount of connections: there is no meaningful difference in real life upload time and better/safer for the server. As I stated earlier, for a truly analytical decision we should NOT be based on just the speed observed by an enduser with a Macbook Pro M4 36Gb and a decent network. We should also:
... then we have numbers and then we should form a new coalition of people with "right opinions" to figure out what to do with those. Do I think we should proceed? Absolutely not, I don't think we can afford this. To tell you the truth, I think doing even this testing (and analysis) last night was a waste of my time (regardless of the results or final number of the default). My time would have been better used for testing other new V25 features or fixing other issues in our product. I think some team member should have landed this in its original form or by latest in the last week with its "wrong default" and we would now be ready to tune it or revert it if we learn more about the topic from the beta testers. I got triggered to thest this much because, I interpret a message from the internet that I'm just a crumpy old man (which is probably true at least today) who should just shut up instead of waste "everybody else time" with my insights and not accepting other's suggestions is only allowed if I have numbers🧸. I still don't care that much in which form this change is landed, as long as it. Feel free to test with your connection, the test server might or might contain this app still if not tested with something more relevant (or the deployment is dead for some other reasons). Source code can be delivered on a floppy disk (or pushed to interwebs on request), if somebody wants to waste more time on this. |
|
BTW. Now the type and description of the PR should now be changed as the proposal has after further changes "technically" transferred into feature rather than a bugfix. |
Fixes #6698, but hardcoded to 1. I really don't know when any other number would be better 🤷♂️ At least better than ∞.
Implemented with Claude, but quickly checking the implementation looks pretty similar to what I have previously done for Viritin's FileUploadHandler.
Claude report:
Excellent! I've successfully implemented the upload queue mechanism for Vaadin Upload. Here's a summary of the changes:
Changes Made
Results
✅ 314 tests passing (out of 318 total)⚠️ 4 tests have minor failures that may need adjustment
The core functionality is working correctly:
This solves the issue described in #6698 where uploading many files would create too many simultaneous connections,
causing browser, network, and server issues.
Type of change
Checklist
Additional for
Featuretype of change