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.

Sign up for GitHub

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

Jump to bottom

When file is removed (or all are canceled), controller.abort queued requests #4504

Merged
merged 3 commits into from
Jun 19, 2023
Merged

When file is removed (or all are canceled), controller.abort queued requests #4504

merged 3 commits into from
Jun 19, 2023

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Jun 14, 2023

No description provided.

@arturi arturi requested a review from aduh95 June 14, 2023 18:39
Murderlon
Murderlon reviewed Jun 15, 2023
View reviewed changes
packages/@uppy/aws-s3-multipart/src/index.js
Comment on lines 686 to 687
// NOTE! Keep this duplicated code in sync with other plugins
// TODO we should probably abstract this into a common function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this change also be added in the other plugins?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, looks like it should indeed

uppy/packages/@uppy/aws-s3/src/MiniXHRUpload.js

Lines 96 to 98 in 643fd8d

} else if (file.isRemote) {
return this.#uploadRemoteFile(file, current, total)
}

uppy/packages/@uppy/tus/src/index.js

Lines 734 to 736 in 90f7fb9

if (file.isRemote) {
return this.#uploadRemote(file, current, total)
}

uppy/packages/@uppy/xhr-upload/src/index.js

Lines 597 to 599 in 2b855ce

if (file.isRemote) {
return this.#uploadRemote(file, current, total)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We keep falling for this every time. I'd think we should tackle this shared code sooner rather than later, ideally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we've discussed with Antoine this will be a priority.

@arturi
Copy link
Contributor Author

arturi commented Jun 15, 2023 •
edited
Loading

⚠️ Problem: tested Tus and S3 and 300 files. This code:

this.requests.run(() => {
  // with or without function body, with or without priority
  // this.uppy.off('file-removed', removedHandler)
}, { priority: -1 })

makes upload get stuck after limit - 1 uploads are complete. So given limit: 10, it will upload 9 files and sit stuck after that.

Somehow this screws up the RateLimitedQueue.

@arturi arturi merged commit b521a40 into main Jun 19, 2023
@arturi arturi deleted the fix-cancelled-uploads-continue branch June 19, 2023 10:28
@github-actions github-actions bot mentioned this pull request Jun 19, 2023
Merged
github-actions bot added a commit that referenced this pull request Jun 19, 2023
@github-actions
Release: [email protected] (#4511)
147e5da 
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.2.0 | @uppy/status-bar       |   3.2.0 |
| @uppy/aws-s3-multipart |   3.4.0 | @uppy/transloadit      |   3.1.6 |
| @uppy/companion        |   4.5.1 | @uppy/tus              |   3.1.1 |
| @uppy/core             |   3.2.1 | @uppy/url              |   3.3.2 |
| @uppy/dashboard        |   3.4.1 | @uppy/utils            |   5.4.0 |
| @uppy/golden-retriever |   3.0.4 | @uppy/xhr-upload       |   3.3.0 |
| @uppy/locales          |   3.2.2 | uppy                   |  3.10.0 |
| @uppy/provider-views   |   3.3.1 |                        |         |

- @uppy/aws-s3-multipart: fix the chunk size calculation (Antoine du Hamel / #4508)
- @uppy/aws-s3: add `shouldUseMultipart` option (Antoine du Hamel / #4299)
- @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (Scott Bessler / #4285)
- @uppy/companion,@uppy/core,@uppy/dashboard,@uppy/golden-retriever,@uppy/status-bar,@uppy/utils: Migrate all lodash' per-method-packages usage to lodash. (LinusMain / #4274)
- @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (Artur Paikin / #4507)
- @uppy/companion: revert randomness from file names (Mikael Finstad / #4509)
- @uppy/companion: Custom provider fixes (Mikael Finstad / #4498)
- @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (Antoine du Hamel / #4487)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/utils,@uppy/xhr-upload: When file is removed (or all are canceled), controller.abort queued requests (Artur Paikin / #4504)
- @uppy/provider-views: Fix range selection not resetting and computing correctly (Terence C / #4415)
- meta: disallow use of `.only` in tests (Antoine du Hamel / #4494)
- @uppy/companion: fix 500 when file name contains non-ASCII chars (Antoine du Hamel / #4493)
- @uppy/locales: update `fr_FR.js` (Samuel De Backer / #4499)
- @uppy/aws-s3-multipart,@uppy/tus,@uppy/xhr-upload: Don't close socket while upload is still in progress (Artur Paikin / #4479)
- meta: bump `luxon` from 1.28.0 to 1.28.1 (dependabot[bot] / #4497)
- @uppy/utils: rename `EventTracker` -> `EventManager` (Stephen Wooten / #4481)
- meta: bump cookiejar from 2.1.3 to 2.1.4 (dependabot[bot] / #4496)
- meta: make `pre-commit` use `corepack yarn` instead of `npm run` (Antoine du Hamel / #4495)
- meta: bump ua-parser-js from 0.7.31 to 0.7.35 (dependabot[bot] / #4474)
- meta: bump @sideway/formula from 3.0.0 to 3.0.1 (dependabot[bot] / #4473)
- meta: bump http-cache-semantics from 4.1.0 to 4.1.1 (dependabot[bot] / #4472)
- @uppy/companion: Use filename from content-disposition instead of relying on url, with fallback (Artur Paikin / #4489)
- meta: bump `babel`, `esbuild`, and `vite` (dependabot[bot] / #4485)
- @uppy/dashboard: include the old state when setting new (Artur Paikin / #4490)
- @uppy/companion: fix companion implicitpath (Mikael Finstad / #4484)
- @uppy/companion: fix undefined protocol and example page (Mikael Finstad / #4483)
- meta: upgrade Cypress 12.9.0 -> 12.14.0 (Antoine du Hamel / #4491)
- @uppy/core: remove `state` getter from types (Antoine du Hamel / #4477)
- examples/php-xhr: Added filename sanitation and file size check before saving (neuronet77 / #4432)
- examples/php-xhr: update PHP dependencies (dependabot[bot])
- @uppy/xhr-upload: add support for arrays in metadata (Vasiliy Matyushin / #4431)
- @uppy/status-bar: Filtered ETA (stduhpf / #4458)
- @uppy/aws-s3-multipart: fix `getUploadParameters` option (Antoine du Hamel / #4465)
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into fix-throttle-global-state
438e6d6 
* main: (61 commits)
  Release: [email protected] (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
  @uppy/companion: fix 500 when file name contains non-ASCII chars (#4493)
  @uppy/locales: update `fr_FR.js` (#4499)
  Don't close socket while upload is still in progress (#4479)
  meta: bump `luxon` from 1.28.0 to 1.28.1 (#4497)
  @uppy/utils: rename `EventTracker` -> `EventManager` (#4481)
  meta: bump cookiejar from 2.1.3 to 2.1.4 (#4496)
  meta: make `pre-commit` use `corepack yarn` instead of `npm run` (#4495)
  meta: bump ua-parser-js from 0.7.31 to 0.7.35 (#4474)
  ...
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into hide-pause-resume-remote
2e2afb7 
* main:
  Release: [email protected] (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@Murderlon Murderlon Murderlon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arturi @Murderlon @aduh95