-
Notifications
You must be signed in to change notification settings - Fork 2k
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
@uppy/aws-s3-multipart: Fix race condition in #uploadParts
#3955
Conversation
The `#uploadParts` function calls itself after any part is uploaded. It also determines which new chunks to upload based on their `state.busy` value. This introduced a race condition, as `state.busy` was being set to false in the XHR event handlers. So if one part were to complete while another part had finished the XHR request, but not yet completed, then an upload for that second part would be started again, despite the fact that the previous upload was still in progress. Multiple uploads for the same part at the same time cause numerous issues, and should never happen. This is especially noticeable when an XHR request fails. `#uploadPart` is wrapped in `#retryable`, so the part will be retried, however, for the entire `retryDelay`, the chunk's `state.busy` value would be false, meaning that if any other part completed, this part would be uploaded again, despite the fact that the upload is already ongoing. To fix this, this commit moves setting `state.busy` to the `before` and `after` functions of the `#retryable` call, so a part will remain `busy` for the entire time it is being uploaded/retried.
Assuming this gets approved, what is the process for backporting a fix to the 2.x release? I need this fix in my application, which is still on uppy 2.x. Should I open a separate PR to the |
@mogzol Thanks for the PR! If approved, we’ll be able to cherry-pick and do a patch release for 2.x as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This has been a bug I couldn't put my finger on where to find it for a while.
It would be nice to test this, but we don't really have the setup for it in e2e so I added a note for it in the PR. Unless you think it's best to test this in unit tests? EDIT: I think we should be able to test this with uppy/packages/@uppy/aws-s3-multipart/src/index.test.js Lines 42 to 119 in 88b08e3
Would you be willing to try to make a test for it? |
I added a test to verify that EDIT: Oops I left a EDIT 2: Fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the effort!
The `#uploadParts` function calls itself after any part is uploaded. It also determines which new chunks to upload based on their `state.busy` value. This introduced a race condition, as `state.busy` was being set to false in the XHR event handlers. So if one part were to complete while another part had finished the XHR request, but not yet completed, then an upload for that second part would be started again, despite the fact that the previous upload was still in progress. Multiple uploads for the same part at the same time cause numerous issues, and should never happen. This is especially noticeable when an XHR request fails. `#uploadPart` is wrapped in `#retryable`, so the part will be retried, however, for the entire `retryDelay`, the chunk's `state.busy` value would be false, meaning that if any other part completed, this part would be uploaded again, despite the fact that the upload is already ongoing. To fix this, this commit moves setting `state.busy` to the `before` and `after` functions of the `#retryable` call, so a part will remain `busy` for the entire time it is being uploaded/retried.
The `#uploadParts` function calls itself after any part is uploaded. It also determines which new chunks to upload based on their `state.busy` value. This introduced a race condition, as `state.busy` was being set to false in the XHR event handlers. So if one part were to complete while another part had finished the XHR request, but not yet completed, then an upload for that second part would be started again, despite the fact that the previous upload was still in progress. Multiple uploads for the same part at the same time cause numerous issues, and should never happen. This is especially noticeable when an XHR request fails. `#uploadPart` is wrapped in `#retryable`, so the part will be retried, however, for the entire `retryDelay`, the chunk's `state.busy` value would be false, meaning that if any other part completed, this part would be uploaded again, despite the fact that the upload is already ongoing. To fix this, this commit moves setting `state.busy` to the `before` and `after` functions of the `#retryable` call, so a part will remain `busy` for the entire time it is being uploaded/retried.
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3-multipart | 2.4.2 | @uppy/robodog | 2.9.3 | | @uppy/compressor | 0.3.2 | uppy | 2.13.3 | | @uppy/utils | 4.1.1 | | | This release marks the transition of Uppy 2.x to maintenance mode. The team is now focused on working Uppy 3.0.0 releases, and plan to not land any new feature on the 2.x release line. We are committed on backporting bug fixes that affects the deprecated Robodog package and its dependencies for at least one year, we encourage Robodog users to migrate away from Robodog to Uppy plugins. - @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / #3955) - meta: fork branch in preparation for LTS (Antoine du Hamel) - @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / #3950) - @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / #3534) - @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / #3947) - e2e: mark tests as flaky (Antoine du Hamel / #3940)
| Package | Version | Package | Version | | ------------------------- | ------------ | ------------------------- | ------------ | | @uppy/audio | 1.0.0-beta.2 | @uppy/progress-bar | 3.0.0-beta.2 | | @uppy/aws-s3 | 3.0.0-beta.3 | @uppy/provider-views | 3.0.0-beta.3 | | @uppy/aws-s3-multipart | 3.0.0-beta.4 | @uppy/react | 3.0.0-beta.4 | | @uppy/box | 2.0.0-beta.2 | @uppy/redux-dev-tools | 3.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.4 | @uppy/remote-sources | 1.0.0-beta.4 | | @uppy/companion-client | 3.0.0-beta.2 | @uppy/screen-capture | 3.0.0-beta.3 | | @uppy/compressor | 1.0.0-beta.3 | @uppy/status-bar | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.4 | @uppy/store-default | 3.0.0-beta.3 | | @uppy/dashboard | 3.0.0-beta.4 | @uppy/store-redux | 3.0.0-beta.3 | | @uppy/drag-drop | 3.0.0-beta.2 | @uppy/svelte | 2.0.0-beta.2 | | @uppy/drop-target | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 | | @uppy/dropbox | 3.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.5 | | @uppy/facebook | 3.0.0-beta.2 | @uppy/tus | 3.0.0-beta.3 | | @uppy/file-input | 3.0.0-beta.2 | @uppy/unsplash | 3.0.0-beta.2 | | @uppy/form | 3.0.0-beta.2 | @uppy/url | 3.0.0-beta.3 | | @uppy/golden-retriever | 3.0.0-beta.2 | @uppy/utils | 5.0.0-beta.1 | | @uppy/google-drive | 3.0.0-beta.2 | @uppy/vue | 1.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.3 | @uppy/webcam | 3.0.0-beta.3 | | @uppy/informer | 3.0.0-beta.3 | @uppy/xhr-upload | 3.0.0-beta.3 | | @uppy/instagram | 3.0.0-beta.2 | @uppy/zoom | 2.0.0-beta.2 | | @uppy/locales | 3.0.0-beta.4 | uppy | 3.0.0-beta.5 | | @uppy/onedrive | 3.0.0-beta.2 | | | - meta: prepare release workflow for beta versions (Antoine du Hamel) - @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / #3978) - @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / #3969) - @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / #3967) - meta: Update CONTRIBUTING.md (Mikael Finstad / #3966) - meta: fix contributing link (Mikael Finstad / #3968) - @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / #3965) - @uppy/utils: Fix webp mimetype (Merlijn Vos / #3961) - @uppy/locales: Add compressor string translation to Japanese locale (kenken / #3963) - meta: Fix statement about cropping images in README.md (Mikael Finstad / #3964) - @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / #3955) - @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / #3951) - @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / #3956) - website: convert all website examples to ESM (Antoine du Hamel / #3957) - @uppy/companion: fix crash if redis disconnects (Mikael Finstad / #3954) - @uppy/companion: upgrade `ws` version (Antoine du Hamel / #3949) - @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / #3897) - @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / #3534) - @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / #3945) - @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / #3950) - @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / #3948) - @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / #3947) - @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / #3907) - @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / #3944) - example: fix aws-companion example (Antoine du Hamel / #3850)
| Package | Version | Package | Version | | ------------------------- | ------------ | ------------------------- | ------------ | | @uppy/audio | 1.0.0-beta.2 | @uppy/progress-bar | 3.0.0-beta.2 | | @uppy/aws-s3 | 3.0.0-beta.3 | @uppy/provider-views | 3.0.0-beta.3 | | @uppy/aws-s3-multipart | 3.0.0-beta.4 | @uppy/react | 3.0.0-beta.4 | | @uppy/box | 2.0.0-beta.2 | @uppy/redux-dev-tools | 3.0.0-beta.2 | | @uppy/companion | 4.0.0-beta.4 | @uppy/remote-sources | 1.0.0-beta.4 | | @uppy/companion-client | 3.0.0-beta.2 | @uppy/screen-capture | 3.0.0-beta.3 | | @uppy/compressor | 1.0.0-beta.3 | @uppy/status-bar | 3.0.0-beta.3 | | @uppy/core | 3.0.0-beta.4 | @uppy/store-default | 3.0.0-beta.3 | | @uppy/dashboard | 3.0.0-beta.4 | @uppy/store-redux | 3.0.0-beta.3 | | @uppy/drag-drop | 3.0.0-beta.2 | @uppy/svelte | 2.0.0-beta.2 | | @uppy/drop-target | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 | | @uppy/dropbox | 3.0.0-beta.2 | @uppy/transloadit | 3.0.0-beta.5 | | @uppy/facebook | 3.0.0-beta.2 | @uppy/tus | 3.0.0-beta.3 | | @uppy/file-input | 3.0.0-beta.2 | @uppy/unsplash | 3.0.0-beta.2 | | @uppy/form | 3.0.0-beta.2 | @uppy/url | 3.0.0-beta.3 | | @uppy/golden-retriever | 3.0.0-beta.2 | @uppy/utils | 5.0.0-beta.1 | | @uppy/google-drive | 3.0.0-beta.2 | @uppy/vue | 1.0.0-beta.2 | | @uppy/image-editor | 2.0.0-beta.3 | @uppy/webcam | 3.0.0-beta.3 | | @uppy/informer | 3.0.0-beta.3 | @uppy/xhr-upload | 3.0.0-beta.3 | | @uppy/instagram | 3.0.0-beta.2 | @uppy/zoom | 2.0.0-beta.2 | | @uppy/locales | 3.0.0-beta.4 | uppy | 3.0.0-beta.5 | | @uppy/onedrive | 3.0.0-beta.2 | | | - meta: prepare release workflow for beta versions (Antoine du Hamel) - @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / transloadit#3978) - @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / transloadit#3969) - @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / transloadit#3967) - meta: Update CONTRIBUTING.md (Mikael Finstad / transloadit#3966) - meta: fix contributing link (Mikael Finstad / transloadit#3968) - @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / transloadit#3965) - @uppy/utils: Fix webp mimetype (Merlijn Vos / transloadit#3961) - @uppy/locales: Add compressor string translation to Japanese locale (kenken / transloadit#3963) - meta: Fix statement about cropping images in README.md (Mikael Finstad / transloadit#3964) - @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / transloadit#3955) - @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / transloadit#3951) - @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / transloadit#3956) - website: convert all website examples to ESM (Antoine du Hamel / transloadit#3957) - @uppy/companion: fix crash if redis disconnects (Mikael Finstad / transloadit#3954) - @uppy/companion: upgrade `ws` version (Antoine du Hamel / transloadit#3949) - @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / transloadit#3897) - @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / transloadit#3534) - @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / transloadit#3945) - @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / transloadit#3950) - @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / transloadit#3948) - @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / transloadit#3947) - @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / transloadit#3907) - @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / transloadit#3944) - example: fix aws-companion example (Antoine du Hamel / transloadit#3850)
The
#uploadParts
function calls itself after any part is uploaded. It alsodetermines which new chunks to upload based on their
state.busy
value. Thisintroduced a race condition, as
state.busy
was being set to false in the XHRevent handlers. So if one part were to complete while another part had finished
the XHR request, but not yet completed, then an upload for that second part
would be started again, despite the fact that the previous upload was still in
progress. Multiple uploads for the same part at the same time cause numerous
issues, and should never happen.
This is especially noticeable when an XHR request fails.
#uploadPart
iswrapped in
#retryable
, so the part will be retried, however, for the entireretryDelay
, the chunk'sstate.busy
value would be false, meaning that ifany other part completed, this part would be uploaded again, despite the fact
that the upload is already ongoing.
To fix this, this commit moves setting
state.busy
to thebefore
andafter
functions of the
#retryable
call, so a part will remainbusy
for the entiretime it is being uploaded/retried.