diff --git a/e2e/cypress/integration/dashboard-aws-multipart.spec.ts b/e2e/cypress/integration/dashboard-aws-multipart.spec.ts index 25ce042a0..3c960f477 100644 --- a/e2e/cypress/integration/dashboard-aws-multipart.spec.ts +++ b/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') + }) }) diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js index e2b1acd05..b732e5718 100644 --- a/packages/@uppy/aws-s3-multipart/src/index.js +++ b/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) { diff --git a/packages/@uppy/status-bar/src/Components.jsx b/packages/@uppy/status-bar/src/Components.jsx index 8667f5d2e..5aa66c11e 100644 --- a/packages/@uppy/status-bar/src/Components.jsx +++ b/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 >