Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x] Fix asset upload concurrency on folder upload #11225

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

daun
Copy link
Contributor

@daun daun commented Dec 6, 2024

The folder upload feature from #10583 has a little flaw: while dropping a bunch of files has always uploaded the files in sequence, dropping a folder will upload all contained files at once. With lots of nested files, this might overwhelm the server and lead to corrupted uploads.

This PR fixes that by only ever allowing a single concurrent upload.

I can imagine this being a config flag to allow e.g. 3 or 5 concurrent uploads if the system can handle it.

Before

Screen Recording 2024-12-06 at 16 33 12

After

Screen Recording 2024-12-06 at 16 46 11

@jasonvarga
Copy link
Member

Interestingly, if you drag a handful of files, they would have already uploaded sequentially.
Seems like they only uploaded all at the same time if you dragged the folder itself.

@daun
Copy link
Contributor Author

daun commented Dec 11, 2024

@jasonvarga Yeah, it took me a while to figure out why exactly. Reading the folder contents occurs in a callback, so the files aren't added all at once. However, the queue implementation assumes that files are added synchronously. That's why it was working fine without checking for ongoing uploads; it simply wasn't necessary.

@jasonvarga jasonvarga merged commit 7ecc2be into statamic:5.x Dec 11, 2024
19 checks passed
@jasonvarga
Copy link
Member

I don't really understand why that matters. this.addFile() still just pushes onto an array.

But, I can't see any harm with this PR. Thanks!

@daun daun deleted the fix/folder-upload-queue branch December 11, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants