From 7f13b43eef047e09ce1c9eabec49127beb59725b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 23 Mar 2022 16:07:52 +0100 Subject: [PATCH 1/4] @uppy/tus: fix double requests sent when rate limiting Fixes: https://github.com/transloadit/uppy/issues/3594 --- e2e/cypress/integration/dashboard-tus.spec.ts | 7 ++--- packages/@uppy/tus/src/index.js | 19 +++---------- packages/@uppy/utils/src/RateLimitedQueue.js | 27 ++++++++++++++++++- yarn.lock | 6 ++--- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/e2e/cypress/integration/dashboard-tus.spec.ts b/e2e/cypress/integration/dashboard-tus.spec.ts index af83172cea..96715508eb 100644 --- a/e2e/cypress/integration/dashboard-tus.spec.ts +++ b/e2e/cypress/integration/dashboard-tus.spec.ts @@ -22,10 +22,7 @@ describe('Dashboard with Tus', () => { cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete') }) - // TODO: figure out why we send a PATCH request while another PATCH is still pending, - // resulting in a 423 upload is locked - // logs: https://gist.github.com/Acconut/d17315d2718d2944aabe2941f268530d - xit('should start exponential backoff when receiving HTTP 429', () => { + it('should start exponential backoff when receiving HTTP 429', () => { cy.get('@file-input').attachFile(['images/cat.jpg', 'images/traffic.jpg']) cy.get('.uppy-StatusBar-actionBtn--upload').click() @@ -39,7 +36,7 @@ describe('Dashboard with Tus', () => { cy.window().then(({ uppy }) => { expect(uppy.getPlugin('Tus').requests.isPaused).to.equal(true) cy.wait('@tus') - cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete') + cy.get('.uppy-StatusBar-statusPrimary', { timeout: 60_000 }).should('contain', 'Complete') }) }) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 5ff2b3388d..5f8d82db01 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -215,6 +215,8 @@ module.exports = class Tus extends BasePlugin { if (typeof opts.onBeforeRequest === 'function') { opts.onBeforeRequest(req) } + + return this.requests.endOfRateLimiting } uploadOptions.onError = (err) => { @@ -259,7 +261,7 @@ module.exports = class Tus extends BasePlugin { resolve(upload) } - uploadOptions.onShouldRetry = (err, retryAttempt, options) => { + uploadOptions.onShouldRetry = (err) => { const status = err?.originalResponse?.getStatus() if (status === 429) { // HTTP 429 Too Many Requests => to avoid the whole download to fail, pause all requests. @@ -270,8 +272,6 @@ module.exports = class Tus extends BasePlugin { } this.requests.rateLimit(next.value) } - queuedRequest.abort() - queuedRequest = this.requests.run(qRequest) } else if (status > 400 && status < 500 && status !== 409) { // HTTP 4xx, the server won't send anything, it's doesn't make sense to retry return false @@ -283,19 +283,7 @@ module.exports = class Tus extends BasePlugin { this.requests.resume() }, { once: true }) } - queuedRequest.abort() - queuedRequest = this.requests.run(qRequest) - } else { - // For a non-4xx error, we can re-queue the request. - setTimeout(() => { - queuedRequest.abort() - queuedRequest = this.requests.run(qRequest) - }, options.retryDelays[retryAttempt]) } - // Aborting the timeout set by tus-js-client to not short-circuit the rate limiting. - // eslint-disable-next-line no-underscore-dangle - queueMicrotask(() => clearTimeout(queuedRequest._retryTimeout)) - // We need to return true here so tus-js-client increments the retryAttempt and do not emit an error event. return true } @@ -325,6 +313,7 @@ module.exports = class Tus extends BasePlugin { this.uploaders[file.id] = upload this.uploaderEvents[file.id] = new EventTracker(this.uppy) + // eslint-disable-next-line prefer-const qRequest = () => { if (!file.isPaused) { upload.start() diff --git a/packages/@uppy/utils/src/RateLimitedQueue.js b/packages/@uppy/utils/src/RateLimitedQueue.js index 56bf3ac502..85b7658c14 100644 --- a/packages/@uppy/utils/src/RateLimitedQueue.js +++ b/packages/@uppy/utils/src/RateLimitedQueue.js @@ -17,6 +17,10 @@ class RateLimitedQueue { #rateLimitingTimer + #rateLimitingPromise = Promise.resolve() + + #rateLimitingPromiseResolve + constructor (limit) { if (typeof limit !== 'number' || limit === 0) { this.limit = Infinity @@ -163,6 +167,12 @@ class RateLimitedQueue { resume () { this.#paused = false clearTimeout(this.#pauseTimer) + + this.#rateLimitingPromiseResolve?.() + // Override the value to allow the promise to be GCed + this.#rateLimitingPromise = Promise.resolve() + this.#rateLimitingPromiseResolve = null + for (let i = 0; i < this.limit; i++) { this.#queueNext() } @@ -173,12 +183,17 @@ class RateLimitedQueue { /** * Freezes the queue for a while or indefinitely. * - * @param {number | null } [duration] Duration for the pause to happen, in milliseconds. + * @param { number | null } [duration] Duration for the pause to happen, in milliseconds. * If omitted, the queue won't resume automatically. */ pause (duration = null) { this.#paused = true clearTimeout(this.#pauseTimer) + + // Override the value to allow the promise to be GCed + this.#rateLimitingPromise = null + this.#rateLimitingPromiseResolve = null + if (duration != null) { this.#pauseTimer = setTimeout(this.#resume, duration) } @@ -222,6 +237,16 @@ class RateLimitedQueue { } get isPaused () { return this.#paused } + + /** + * @returns {Promise} Promise that resolves when the queue is no longer paused. + */ + get endOfRateLimiting () { + this.#rateLimitingPromise ??= new Promise(resolve => { + this.#rateLimitingPromiseResolve = resolve + }) + return this.#rateLimitingPromise + } } module.exports = { diff --git a/yarn.lock b/yarn.lock index 7299d64323..b02fff3c47 100644 --- a/yarn.lock +++ b/yarn.lock @@ -16653,8 +16653,8 @@ __metadata: linkType: hard "cypress@npm:^9.0.0": - version: 9.5.1 - resolution: "cypress@npm:9.5.1" + version: 9.5.2 + resolution: "cypress@npm:9.5.2" dependencies: "@cypress/request": ^2.88.10 "@cypress/xvfb": ^1.2.4 @@ -16700,7 +16700,7 @@ __metadata: yauzl: ^2.10.0 bin: cypress: bin/cypress - checksum: 96efebb8bfca52f214f793856a7eb40a6eb4a7bc06e5d7c17c7378cec8e3e3975d5777147066d04d95d88b99d4052bbf8749d96b4a25d3f79cc65e88e93f703d + checksum: d19a183df25adcb87ba4914fb177a2b8bf2f004c8364fa21abb6bf66f2462d6f586bf09b58480e804f6b499281d931712a3e262f09880859e3170d513c5c9227 languageName: node linkType: hard From 65fba701cc48a2da90a40466897f5202895ae127 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 23 Mar 2022 19:48:32 +0100 Subject: [PATCH 2/4] fixup! requeue requests instead --- packages/@uppy/tus/src/index.js | 17 +++++++++++- packages/@uppy/utils/src/RateLimitedQueue.js | 27 +------------------- 2 files changed, 17 insertions(+), 27 deletions(-) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 5f8d82db01..21e01b202d 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -216,7 +216,20 @@ module.exports = class Tus extends BasePlugin { opts.onBeforeRequest(req) } - return this.requests.endOfRateLimiting + if (queuedRequest == null) { + let done + queuedRequest = this.requests.run(() => { + if (file.isPaused) { + queuedRequest.abort() + } + done() + return () => {} + }) + return new Promise(resolve => { // eslint-disable-line no-shadow + done = resolve + }) + } + return undefined } uploadOptions.onError = (err) => { @@ -284,6 +297,8 @@ module.exports = class Tus extends BasePlugin { }, { once: true }) } } + queuedRequest.abort() + queuedRequest = null return true } diff --git a/packages/@uppy/utils/src/RateLimitedQueue.js b/packages/@uppy/utils/src/RateLimitedQueue.js index 85b7658c14..56bf3ac502 100644 --- a/packages/@uppy/utils/src/RateLimitedQueue.js +++ b/packages/@uppy/utils/src/RateLimitedQueue.js @@ -17,10 +17,6 @@ class RateLimitedQueue { #rateLimitingTimer - #rateLimitingPromise = Promise.resolve() - - #rateLimitingPromiseResolve - constructor (limit) { if (typeof limit !== 'number' || limit === 0) { this.limit = Infinity @@ -167,12 +163,6 @@ class RateLimitedQueue { resume () { this.#paused = false clearTimeout(this.#pauseTimer) - - this.#rateLimitingPromiseResolve?.() - // Override the value to allow the promise to be GCed - this.#rateLimitingPromise = Promise.resolve() - this.#rateLimitingPromiseResolve = null - for (let i = 0; i < this.limit; i++) { this.#queueNext() } @@ -183,17 +173,12 @@ class RateLimitedQueue { /** * Freezes the queue for a while or indefinitely. * - * @param { number | null } [duration] Duration for the pause to happen, in milliseconds. + * @param {number | null } [duration] Duration for the pause to happen, in milliseconds. * If omitted, the queue won't resume automatically. */ pause (duration = null) { this.#paused = true clearTimeout(this.#pauseTimer) - - // Override the value to allow the promise to be GCed - this.#rateLimitingPromise = null - this.#rateLimitingPromiseResolve = null - if (duration != null) { this.#pauseTimer = setTimeout(this.#resume, duration) } @@ -237,16 +222,6 @@ class RateLimitedQueue { } get isPaused () { return this.#paused } - - /** - * @returns {Promise} Promise that resolves when the queue is no longer paused. - */ - get endOfRateLimiting () { - this.#rateLimitingPromise ??= new Promise(resolve => { - this.#rateLimitingPromiseResolve = resolve - }) - return this.#rateLimitingPromise - } } module.exports = { From 178046d73aa0665707adff87f1872067a4d465cf Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 23 Mar 2022 20:17:48 +0100 Subject: [PATCH 3/4] fix `cannot read abort of null` error --- packages/@uppy/tus/src/index.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index 21e01b202d..ddd4820fbc 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -218,16 +218,17 @@ module.exports = class Tus extends BasePlugin { if (queuedRequest == null) { let done + const p = new Promise((res) => { // eslint-disable-line promise/param-names + done = res + }) queuedRequest = this.requests.run(() => { - if (file.isPaused) { + if (file.isPaused && queuedRequest != null) { queuedRequest.abort() } done() return () => {} }) - return new Promise(resolve => { // eslint-disable-line no-shadow - done = resolve - }) + return p } return undefined } @@ -241,7 +242,7 @@ module.exports = class Tus extends BasePlugin { } this.resetUploaderReferences(file.id) - queuedRequest.abort() + queuedRequest?.abort() this.uppy.emit('upload-error', file, err) @@ -297,7 +298,7 @@ module.exports = class Tus extends BasePlugin { }, { once: true }) } } - queuedRequest.abort() + queuedRequest?.abort() queuedRequest = null return true } @@ -353,37 +354,36 @@ module.exports = class Tus extends BasePlugin { queuedRequest = this.requests.run(qRequest) this.onFileRemove(file.id, (targetFileID) => { - queuedRequest.abort() + queuedRequest?.abort() this.resetUploaderReferences(file.id, { abort: !!upload.url }) resolve(`upload ${targetFileID} was removed`) }) this.onPause(file.id, (isPaused) => { + queuedRequest?.abort() if (isPaused) { // Remove this file from the queue so another file can start in its place. - queuedRequest.abort() upload.abort() } else { // Resuming an upload should be queued, else you could pause and then // resume a queued upload to make it skip the queue. - queuedRequest.abort() queuedRequest = this.requests.run(qRequest) } }) this.onPauseAll(file.id, () => { - queuedRequest.abort() + queuedRequest?.abort() upload.abort() }) this.onCancelAll(file.id, () => { - queuedRequest.abort() + queuedRequest?.abort() this.resetUploaderReferences(file.id, { abort: !!upload.url }) resolve(`upload ${file.id} was canceled`) }) this.onResumeAll(file.id, () => { - queuedRequest.abort() + queuedRequest?.abort() if (file.error) { upload.abort() } From cb51378f8b28a161abcf0f69b44a647eb1ea2023 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 24 Mar 2022 11:23:48 +0100 Subject: [PATCH 4/4] don't use null queuedRequest, it's confusing and less performant --- packages/@uppy/tus/src/index.js | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/@uppy/tus/src/index.js b/packages/@uppy/tus/src/index.js index ddd4820fbc..228509388a 100644 --- a/packages/@uppy/tus/src/index.js +++ b/packages/@uppy/tus/src/index.js @@ -216,13 +216,14 @@ module.exports = class Tus extends BasePlugin { opts.onBeforeRequest(req) } - if (queuedRequest == null) { + if (Object.hasOwn(queuedRequest, 'shouldBeRequeued')) { + if (!queuedRequest.shouldBeRequeued) return Promise.reject() let done const p = new Promise((res) => { // eslint-disable-line promise/param-names done = res }) queuedRequest = this.requests.run(() => { - if (file.isPaused && queuedRequest != null) { + if (file.isPaused) { queuedRequest.abort() } done() @@ -242,7 +243,7 @@ module.exports = class Tus extends BasePlugin { } this.resetUploaderReferences(file.id) - queuedRequest?.abort() + queuedRequest.abort() this.uppy.emit('upload-error', file, err) @@ -298,8 +299,19 @@ module.exports = class Tus extends BasePlugin { }, { once: true }) } } - queuedRequest?.abort() - queuedRequest = null + queuedRequest.abort() + queuedRequest = { + shouldBeRequeued: true, + abort () { + this.shouldBeRequeued = false + }, + done () { + throw new Error('Cannot mark a queued request as done: this indicates a bug') + }, + fn () { + throw new Error('Cannot run a queued request: this indicates a bug') + }, + } return true } @@ -354,13 +366,13 @@ module.exports = class Tus extends BasePlugin { queuedRequest = this.requests.run(qRequest) this.onFileRemove(file.id, (targetFileID) => { - queuedRequest?.abort() + queuedRequest.abort() this.resetUploaderReferences(file.id, { abort: !!upload.url }) resolve(`upload ${targetFileID} was removed`) }) this.onPause(file.id, (isPaused) => { - queuedRequest?.abort() + queuedRequest.abort() if (isPaused) { // Remove this file from the queue so another file can start in its place. upload.abort() @@ -372,18 +384,18 @@ module.exports = class Tus extends BasePlugin { }) this.onPauseAll(file.id, () => { - queuedRequest?.abort() + queuedRequest.abort() upload.abort() }) this.onCancelAll(file.id, () => { - queuedRequest?.abort() + queuedRequest.abort() this.resetUploaderReferences(file.id, { abort: !!upload.url }) resolve(`upload ${file.id} was canceled`) }) this.onResumeAll(file.id, () => { - queuedRequest?.abort() + queuedRequest.abort() if (file.error) { upload.abort() }