@uppy/companion-client: prevent preflight race condition #4182
Conversation
This file contains 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
that caused double OPTIONS to be sent for each reqeust Also simplify, deduplicate and async/awaitify code and fix lint
Murderlon
approved these changes
Nov 3, 2022
| } | ||
|
|
||
| // todo pull out into core instead? |
There was a problem hiding this comment.
core has no knowledge of whether you use Companion or not so I don't think it belongs there
There was a problem hiding this comment.
is there any other place where we can store state about a session that is not per-file, but per Uppy instance? I don't like global variables like this. It will leak memory if companionUrl changes. Although maybe unlikely, what if someone adds some trickery to their companionUrl, where there's a new companionUrl for every upload.
There was a problem hiding this comment.
How about this.getPluginState/this.setPluginState?
There was a problem hiding this comment.
That's a good idea, but we would have to send this.getPluginState and this.setPluginState into every RequestClient instance. Not sure if that's the cleanest way to do it.
Murderlon
changed the title
aduh95
reviewed
Nov 3, 2022
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
reviewed
Nov 3, 2022
mifi
commented
Nov 3, 2022
aduh95
reviewed
Nov 3, 2022
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
feel free to review my last commit, then merge this |
Murderlon
approved these changes
Nov 7, 2022
aduh95
reviewed
Nov 7, 2022
mifi
commented
Nov 8, 2022
mifi
commented
Nov 8, 2022
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
Merged
github-actions bot
added a commit
that referenced
this pull request
Nov 10, 2022
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/angular | 0.5.0 | @uppy/image-editor | 2.1.0 | | @uppy/aws-s3-multipart | 3.1.0 | @uppy/locales | 3.0.4 | | @uppy/companion | 4.1.0 | @uppy/tus | 3.0.5 | | @uppy/companion-client | 3.1.0 | @uppy/utils | 5.1.0 | | @uppy/dashboard | 3.2.0 | uppy | 3.3.0 | - @uppy/companion: change default S3 expiry from 300 to 800 seconds (Merlijn Vos / #4206) - @uppy/dashboard: Single file mode (Artur Paikin / #4188) - @uppy/locales: Fix UZ locale (Merlijn Vos / #4178) - @uppy/utils: update typings for `RateLimitedQueue` (Antoine du Hamel / #4204) - @uppy/aws-s3-multipart: empty the queue when pausing (Antoine du Hamel / #4203) - @uppy/image-editor: add checkered background (Livia Medeiros / #4194) - @uppy/aws-s3-multipart: refactor rate limiting approach (Antoine du Hamel / #4187) - @uppy/companion: send expiry time along side S3 signed requests (Antoine du Hamel / #4202) - @uppy/companion-client: add support for `AbortSignal` (Antoine du Hamel / #4201) - @uppy/companion-client: prevent preflight race condition (Mikael Finstad / #4182) - @uppy/aws-s3-multipart: change limit to 6 (Antoine du Hamel / #4199) - @uppy/utils: add `cause` support for `AbortError`s (Antoine du Hamel / #4198) - meta: Fix bad example for setFileState (Tim Whitney / #4191) - meta: Update code example for getFiles (Tim Whitney / #4189) - meta: Fix issue with outdated comment. (Tim Whitney / #4192) - @uppy/aws-s3-multipart: remove unused `timeout` option (Antoine du Hamel / #4186) - meta: Remove dollar sign from command for easier copy/pasting (Youssef Victor / #4180) - @uppy/aws-s3-multipart,@uppy/tus: fix `Timed out waiting for socket` (Antoine du Hamel / #4177) - meta: Add note about facebook approval (Mikael Finstad / #4172) - meta: add a manual deploy for website (Antoine du Hamel / #4171)
Murderlon
added a commit
that referenced
this pull request
Dec 1, 2022
* main: (110 commits) @uppy/aws-s3-multipart: handle slow connections better (#4213) @uppy/companion-client: treat `*` the same as missing header (#4221) @uppy/utils: fix types (#4212) @uppy/companion: send expire info for non-multipart uploads (#4214) docs: fix `allowedMetaFields` documentation (#4216) meta: add more bundlers for automated testing (#4100) @uppy/aws-s3-multipart: Fix typo in url check (#4211) meta: use current version of packages when testing bundlers (#4208) meta: do not use the set-output command in workflows (#4175) Release: uppy@3.3.0 (#4207) Companion: change default S3 expiry from 300 to 800 seconds (#4206) Dashboard: Single file mode (#4188) Fix UZ locale (#4178) @uppy/utils: update typings for `RateLimitedQueue` (#4204) @uppy/aws-s3-multipart: empty the queue when pausing (#4203) image-editor: add checkered background (#4194) @uppy/aws-s3-multipart: refactor rate limiting approach (#4187) @uppy/companion: send expiry time along side S3 signed requests (#4202) @uppy/companion-client: add support for `AbortSignal` (#4201) @uppy/companion-client: prevent preflight race condition (#4182) ...
HeavenFox
pushed a commit
to docsend/uppy
that referenced
this pull request
Jun 27, 2023
| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/angular | 0.5.0 | @uppy/image-editor | 2.1.0 | | @uppy/aws-s3-multipart | 3.1.0 | @uppy/locales | 3.0.4 | | @uppy/companion | 4.1.0 | @uppy/tus | 3.0.5 | | @uppy/companion-client | 3.1.0 | @uppy/utils | 5.1.0 | | @uppy/dashboard | 3.2.0 | uppy | 3.3.0 | - @uppy/companion: change default S3 expiry from 300 to 800 seconds (Merlijn Vos / transloadit#4206) - @uppy/dashboard: Single file mode (Artur Paikin / transloadit#4188) - @uppy/locales: Fix UZ locale (Merlijn Vos / transloadit#4178) - @uppy/utils: update typings for `RateLimitedQueue` (Antoine du Hamel / transloadit#4204) - @uppy/aws-s3-multipart: empty the queue when pausing (Antoine du Hamel / transloadit#4203) - @uppy/image-editor: add checkered background (Livia Medeiros / transloadit#4194) - @uppy/aws-s3-multipart: refactor rate limiting approach (Antoine du Hamel / transloadit#4187) - @uppy/companion: send expiry time along side S3 signed requests (Antoine du Hamel / transloadit#4202) - @uppy/companion-client: add support for `AbortSignal` (Antoine du Hamel / transloadit#4201) - @uppy/companion-client: prevent preflight race condition (Mikael Finstad / transloadit#4182) - @uppy/aws-s3-multipart: change limit to 6 (Antoine du Hamel / transloadit#4199) - @uppy/utils: add `cause` support for `AbortError`s (Antoine du Hamel / transloadit#4198) - meta: Fix bad example for setFileState (Tim Whitney / transloadit#4191) - meta: Update code example for getFiles (Tim Whitney / transloadit#4189) - meta: Fix issue with outdated comment. (Tim Whitney / transloadit#4192) - @uppy/aws-s3-multipart: remove unused `timeout` option (Antoine du Hamel / transloadit#4186) - meta: Remove dollar sign from command for easier copy/pasting (Youssef Victor / transloadit#4180) - @uppy/aws-s3-multipart,@uppy/tus: fix `Timed out waiting for socket` (Antoine du Hamel / transloadit#4177) - meta: Add note about facebook approval (Mikael Finstad / transloadit#4172) - meta: add a manual deploy for website (Antoine du Hamel / transloadit#4171)
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.
...that caused double OPTIONS to be sent for each inital reqeust
Also simplify, deduplicate and async/awaitify code and fix lint.
This race condition caused the RequestClient to send X redundant OPTIONS requests immediately when starting an upload, where X is the number of files uploaded. This affected all Companion-related uploads: s3, s3-multipart, google drive etc. Note that after the initial requests, it would not send double OPTIONS (such as multipart uploading individual parts, because it would be cached by then).
This cache would be cleared for every upload "session".
After this PR, we only send OPTIONS once, then wait for the response before continuing. We globally cache the allowed headers based on
companionUrl, so even in subsequent upload sessions, it will not send any more of these extraneous OPTIONS requeststip for easier review: ignore whitespace