Conversation
Upload files in batches for faster uploading
Many requests to add files coming at the same time can cause duplicate folders to be created. In some cases, SQL exceptions can throw, and cause the application to hang. Add Semaphore to process files on a single thread.
|
Hi there @matthewcare, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
|
H @matthewcare , thanks for this PR, it's a nice feature/fix 👍 Let me check with HQ with your questions and I'll get back to you asap. Cheers! |
|
@mikecp |
|
Hi @matthewcare , Woops, no, and I kind of lost track on this one, sorry 😬 I'll ping them again but promise to come back to you much quickly! Cheers and thanks for your patience! |
|
@matthewcare We ended up deciding that locking was not the way to go on this, instead we wrapped the whole Transaction in a scope instead 👍 Can you try this approach out ? 🙏 |
|
@matthewcare when you do make updates, please note that I've changed the base branch to You should be able to delete your local branch and pull it down from your fork to get the updated version. |
As suggested in the PR comments, replaced Semaphore with ICoreScopeProvider
|
@nul800sebastiaan @Zeegaan I've pushed the code so you can have a look yourself to see if I did something wrong? |
|
@matthewcare No I think you did it right. |
|
@Zeegaan |
|
@matthewcare I think the semaphore approach with a TODO is just fine, so we finally can get this in💪 |
|
@Zeegaan
|
|
@matthewcare No sorry, that totally slipped my mind 😅 |
This reverts commit 228e101.
Out of scope of the PR, but this should be configurable in appsettings
|
@matthewcare When testing, my images just get stuck at 100% 🤔 |
|
@Zeegaan |
|
@Zeegaan |
|
Hmmm still happens for me, even after clearing npm caches and everything, I'll try with a fresh clone to be 100% sure 🤔 |
|
You definitely rebuilt the solution right? |
|
@matthewcare Yup, this is me on a fresh clone, fresh npm install & npm run build etc. |
|
@Zeegaan Can you try without the |
|
@matthewcare Yea backoffice continues to work, so I can just navigate away from the page 👍 Same result without the huge image aswell 🤔 No console errors either 😅 |
|
@Zeegaan and I can't get it to break. |
|
@matthewcare I will test when i get home later this afternoon on my personal computer 👍 |
|
@matthewcare I can also reproduce at home, so that's at least two machines 😅 |
|
@Zeegaan |
|
@matthewcare Ah yea that was a bit vague LOL. |
|
@Zeegaan |
|
@Zeegaan I've done a completely fresh checkout, and tried running in a variety of configurations I cannot replicate the issue you're having, though you having the issue on two computers is definitely concerning. I'll try and get this set up on a second computer also and give it another go there. EDIT: |
|
@mikecp Can you help us out here 🙈 It might be me mocking things up |
|
I've also now installed VS on a new laptop, and ran the project as a "this project has never even been run on the laptop before" check, and still couldn't replicate your issue @Zeegaan. Something strange is definitely happening for you (or me). |
|
In the current state of this PR I have taken @Zeegaan's zip file and uploaded it as a full folder. I also get stuck at 100% for a few files and I have a console error that might help:
|
|
Not sure why |
|
The console errors are related to PR #13345 (an issue with Angular 1.6+ and also a different issue regarding max file upload limits not working properly anymore). Hmm, I wonder if it's because you have mega fast internet and the file size limit exception is breaking the rest of the files from running... When I test the large image is always last to upload. Though @Zeegaan confirmed that the uploads still got stuck with the large image omitted from the upload. |
I think it's because the file is rejected for being too large, but for some reason still gets through to the controller somehow |
Apply PR 13345 to resolve "freezing" issue caused by unhandled errors
|
@Zeegaan @nul800sebastiaan I've applied the fix to errors not being handled correctly (ported from PR #13345) and that should resolve the issue! Could you try again? |
|
@matthewcare I did manage to get it to work now 🎉 |
|
@Zeegaan I'll take another look and see if it might be related to the error Seb mentioned, though if that's the issue then there might be a few more PRs to open before this feature can be added :( |
|
@matthewcare I love it, there more issues we can find and know about, the better! 💪 |
If you drag files into the dropzone whilst other files are uploading then the "total" count is incorrect.
|
@Zeegaan Hopefully that's what you did, and there shouldn't be any further issues :) |
|
@matthewcare Yep that did it! 🎉 |
Zeegaan
added
release/11.1.0
category/performance
|
@Zeegaan |
|
Woops, sorry I missed you ping 2 weeks ago @Zeegaan ... |
4 participants
Prerequisites
When uploading a whole folder of media, you have to wait for them to process one by one.
This PR adds functionality to allow multiple file uploads concurrently.
With the media I was testing with, this resulted in the folder uploading 30% faster, though results may vary depending on filesize / upload speed.
Whilst testing this functionality I found that if multiple requests hit the server at the same time, there can be concurrency issues resulting in duplicate folders being created, and in some instances, the application would throw SQL exceptions, and hang.
I imagine this issue is also present when multiple content editors upload things at the same time as well, though probably isn't a common bug.
Adding Semaphore to restrict the execution to a single thread and resolve the concurrency issues. I'm not 100% if this is a preferred approach, so please correct me / the code if you think there is a better way, or if there is already something in the codebase to handle this.
I'm not sure if I should make the batch size configurable in the appsettings, or if leaving it at a constant number is acceptable. Let me know the preference here.
Testing
Upload a folder of media, and they will upload in batches of 10. Uploading large files, or many files on a slow connection will better demonstrate this.