fix: stop a cancelled upload from starting the ones behind it - #213
Open
yash-sangwan wants to merge 1 commit into
Open
fix: stop a cancelled upload from starting the ones behind it#213yash-sangwan wants to merge 1 commit into
yash-sangwan wants to merge 1 commit into
Conversation
Cancelling a folder pumped the upload queue between files, so the slots the running uploads gave up went to files about to be cancelled too. It also leaked a multipart upload per file cancelled mid-create, and wrote one store update per file, which is what froze the tab.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
Author
|
@Rakesh-46-VR am dicy on this, havn't tested, it will definitely take time |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the draft issue about POST and OPTIONS calls still going out after a cancel.
What was wrong
Cancelling a folder cancels its files one by one. While that is happening the queue is still full of files that are about to be cancelled too, and the old code refilled the queue after every single cancel. So the free slots went straight to files that were seconds away from being cancelled themselves. Each one started for real, which is the POST and the OPTIONS you saw in the network tab.
Cancelling 3000 files started 1498 uploads that nobody wanted.
Two more things came out of the same bug:
Those uploads were started and cancelled so fast that S3 had not replied yet with the upload id. Without that id the cancel could not tell S3 to throw the upload away, so it stayed in the bucket as an incomplete multipart upload. You cannot see these in any file listing but AWS still charges for them.
The bigger reason the page froze was not the network at all. Every cancelled file wrote to the upload store on its own, and every write copied the whole list of uploads. With 3000 files that is 3000 copies of a 3000 item list. Measured, it blocked the browser for 9.3 seconds. Now it is one write for the whole batch and the same 3000 files take 9 milliseconds.
How to test this
You need a decent number of files, a few hundred at least. The bug gets more obvious the more files there are.
What you should see now:
?uploads=after you hit cancel, and no OPTIONS calls either. The only new requests should be the aborts for the two or three files that were genuinely uploading at that moment.Also worth checking that nothing normal broke:
If you have access to the bucket, a nice extra check is
aws s3api list-multipart-uploads --bucket <your bucket>after cancelling a big folder. It should come back empty or close to it. Before this fix it would list hundreds of leftover uploads.What changed
s3-api/src/utils/uploadManager.tsandsignedUrlUploadManager.ts: refilling the queue after a cancel now waits until the caller has finished cancelling, so only work nobody cancelled can start.s3-api/src/utils/multipartUploader.ts: if the cancel arrives before S3 hands back the upload id, the upload aborts itself once the id shows up. It also skips opening an upload at all if the cancel got there first.frontend/src/features/upload/context/upload-context.tsxandstores/use-upload-store.ts: upload events are collected for a tick and written to the store in one go instead of one write per file.15 new tests, and all 2039 existing tests still pass.