@uppy/tus: fix double requests sent when rate limiting (#3595)

Fixes: #3594
transloadit · Mar 24, 2022 · f16359b · f16359b
1 parent 2110741
commit f16359b
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 25 deletions.
diff --git 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>('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
@@ -215,6 +215,23 @@ module.exports = class Tus extends BasePlugin {
         if (typeof opts.onBeforeRequest === 'function') {
           opts.onBeforeRequest(req)
         }
+
+        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.abort()
+            }
+            done()
+            return () => {}
+          })
+          return p
+        }
+        return undefined
       }
 
       uploadOptions.onError = (err) => {
@@ -259,7 +276,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 +287,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 +298,20 @@ 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.
+        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
       }
 
@@ -325,6 +341,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()
@@ -355,14 +372,13 @@ module.exports = class Tus extends BasePlugin {
       })
 
       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)
         }
       })

diff --git 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