@uppy/aws-s3-multipart: make retries more robust (#4424)

e2e/cypress/integration/dashboard-aws-multipart.spec.ts

@@ -14,4 +14,74 @@ describe('Dashboard with @uppy/aws-s3-multipart', () => {
     cy.wait(['@post', '@get', '@put'])
     cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
   })
+
+  it('should handle retry request gracefully',  () => {
+    cy.get('@file-input').selectFile('cypress/fixtures/images/cat.jpg', { force:true })
+
+    cy.intercept('POST', '/s3/multipart', { forceNetworkError: true, times: 1 }).as('createMultipartUpload-fails')
+    cy.get('.uppy-StatusBar-actionBtn--upload').click()
+    cy.wait(['@createMultipartUpload-fails'])
+    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Upload failed')
+
+    cy.intercept('POST', '/s3/multipart', { statusCode: 200, times: 1, body: JSON.stringify({ key:'mocked-key-attempt1', uploadId:'mocked-uploadId-attempt1' }) }).as('createMultipartUpload-attempt1')
+    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt1/1?key=mocked-key-attempt1', { forceNetworkError: true }).as('signPart-fails')
+    cy.get('.uppy-StatusBar-actions > .uppy-c-btn').click()
+    cy.wait(['@createMultipartUpload-attempt1', '@signPart-fails'])
+    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Upload failed')
+
+    cy.intercept('POST', '/s3/multipart', { statusCode: 200, times: 1, body: JSON.stringify({ key:'mocked-key-attempt2', uploadId:'mocked-uploadId-attempt2' }) }).as('createMultipartUpload-attempt2')
+    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt2/1?key=mocked-key-attempt2', {
+      statusCode: 200,
+      headers: {
+        ETag: 'W/"222-GXE2wLoMKDihw3wxZFH1APdUjHM"',
+      },
+      body: JSON.stringify({ url:'/put-fail', expires:8 }),
+    }).as('signPart-toFail')
+    cy.intercept('PUT', '/put-fail', { forceNetworkError: true }).as('put-fails')
+    cy.get('.uppy-StatusBar-actions > .uppy-c-btn').click()
+    cy.wait(['@createMultipartUpload-attempt2', '@signPart-toFail', ...Array(5).fill('@put-fails')], { timeout: 10_000 })
+    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Upload failed')
+
+    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt2/1?key=mocked-key-attempt2', {
+      statusCode: 200,
+      headers: {
+        ETag: 'ETag-attempt2',
+      },
+      body: JSON.stringify({ url:'/put-success-attempt2', expires:8 }),
+    }).as('signPart-attempt2')
+    cy.intercept('PUT', '/put-success-attempt2', {
+      statusCode: 200,
+      headers: {
+        ETag: 'ETag-attempt2',
+      },
+    }).as('put-attempt2')
+    cy.intercept('POST', '/s3/multipart/mocked-uploadId-attempt2/complete?key=mocked-key-attempt2', { forceNetworkError: true }).as('completeMultipartUpload-fails')
+    cy.get('.uppy-StatusBar-actions > .uppy-c-btn').click()
+    cy.wait(['@createMultipartUpload-attempt2', '@signPart-attempt2', '@put-attempt2', '@completeMultipartUpload-fails'])
+    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Upload failed')
+
+    cy.intercept('POST', '/s3/multipart', { statusCode: 200, times: 1, body: JSON.stringify({ key:'mocked-key-attempt3', uploadId:'mocked-uploadId-attempt3' }) }).as('createMultipartUpload-attempt3')
+    cy.intercept('GET', '/s3/multipart/mocked-uploadId-attempt3/1?key=mocked-key-attempt3', {
+      statusCode: 200,
+      headers: {
+        ETag: 'ETag-attempt3',
+      },
+      body: JSON.stringify({ url:'/put-success-attempt3', expires:8 }),
+    }).as('signPart-attempt3')
+    cy.intercept('PUT', '/put-success-attempt3', {
+      statusCode: 200,
+      headers: {
+        ETag: 'ETag-attempt3',
+      },
+    }).as('put-attempt3')
+    cy.intercept('POST', '/s3/multipart/mocked-uploadId-attempt3/complete?key=mocked-key-attempt3', {
+      statusCode: 200,
+      body: JSON.stringify({
+        location: 'someLocation',
+      }),
+    }).as('completeMultipartUpload-attempt3')
+    cy.get('.uppy-StatusBar-actions > .uppy-c-btn').click()
+    cy.wait(['@createMultipartUpload-attempt3', '@signPart-attempt3', '@put-attempt3', '@completeMultipartUpload-attempt3'])
+    cy.get('.uppy-StatusBar-statusPrimary').should('contain', 'Complete')
+  })
 })

packages/@uppy/aws-s3-multipart/src/index.js

@@ -140,9 +140,17 @@ class HTTPCommunicationQueue {
   }
 
   async getUploadId (file, signal) {
-    const cachedResult = this.#cache.get(file.data)
-    if (cachedResult != null) {
-      return cachedResult
+    let cachedResult
+    // As the cache is updated asynchronously, there could be a race condition
+    // where we just miss a new result so we loop here until we get nothing back,
+    // at which point it's out turn to create a new cache entry.
+    while ((cachedResult = this.#cache.get(file.data)) != null) {
+      try {
+        return await cachedResult
+      } catch {
+        // In case of failure, we want to ignore the cached error.
+        // At this point, either there's a new cached value, or we'll exit the loop a create a new one.
+      }
     }
 
     const promise = this.#createMultipartUpload(file, signal)
@@ -157,27 +165,43 @@ class HTTPCommunicationQueue {
       signal.removeEventListener('abort', abortPromise)
       this.#setS3MultipartState(file, result)
       this.#cache.set(file.data, result)
-    }, () => { signal.removeEventListener('abort', abortPromise) })
+    }, () => {
+      signal.removeEventListener('abort', abortPromise)
+      this.#cache.delete(file.data)
+    })
 
     return promise
   }
 
   async abortFileUpload (file) {
     const result = this.#cache.get(file.data)
-    if (result != null) {
+    if (result == null) {
       // If the createMultipartUpload request never was made, we don't
       // need to send the abortMultipartUpload request.
-      await this.#abortMultipartUpload(file, await result)
+      return
     }
+    let awaitedResult
+    try {
+      awaitedResult = await result
+    } catch {
+      // If the cached result rejects, there's nothing to abort.
+      return
+    }
+    await this.#abortMultipartUpload(file, awaitedResult)
   }
 
   async uploadFile (file, chunks, signal) {
     throwIfAborted(signal)
     const { uploadId, key } = await this.getUploadId(file, signal)
     throwIfAborted(signal)
-    const parts = await Promise.all(chunks.map((chunk, i) => this.uploadChunk(file, i + 1, chunk, signal)))
-    throwIfAborted(signal)
-    return this.#sendCompletionRequest(file, { key, uploadId, parts, signal }).abortOn(signal)
+    try {
+      const parts = await Promise.all(chunks.map((chunk, i) => this.uploadChunk(file, i + 1, chunk, signal)))
+      throwIfAborted(signal)
+      return await this.#sendCompletionRequest(file, { key, uploadId, parts, signal }).abortOn(signal)
+    } catch (err) {
+      this.#cache.delete(file.data)
+      throw err
+    }
   }
 
   async resumeUploadFile (file, chunks, signal) {

packages/@uppy/status-bar/src/Components.jsx

@@ -57,7 +57,7 @@ function RetryBtn (props) {
       type="button"
       className="uppy-u-reset uppy-c-btn uppy-StatusBar-actionBtn uppy-StatusBar-actionBtn--retry"
       aria-label={i18n('retryUpload')}
-      onClick={() => uppy.retryAll()}
+      onClick={() => uppy.retryAll().catch(() => { /* Error reported and handled via an event */ })}
       data-uppy-super-focusable
     >
       <svg