From 3b878afb56ea1297e34e3660406e5b7dc1b28aea Mon Sep 17 00:00:00 2001
From: Morgan Zolob <mzolob@gmail.com>
Date: Thu, 4 Aug 2022 14:48:35 -0700
Subject: [PATCH] @uppy/aws-s3-multipart: Fix race condition in `#uploadParts`

The `#uploadParts` function calls itself after any part is uploaded. It also
determines which new chunks to upload based on their `state.busy` value. This
introduced a race condition, as `state.busy` was being set to false in the XHR
event handlers. So if one part were to complete while another part had finished
the XHR request, but not yet completed, then an upload for that second part
would be started again, despite the fact that the previous upload was still in
progress. Multiple uploads for the same part at the same time cause numerous
issues, and should never happen.

This is especially noticeable when an XHR request fails. `#uploadPart` is
wrapped in `#retryable`, so the part will be retried, however, for the entire
`retryDelay`, the chunk's `state.busy` value would be false, meaning that if
any other part completed, this part would be uploaded again, despite the fact
that the upload is already ongoing.

To fix this, this commit moves setting `state.busy` to the `before` and `after`
functions of the `#retryable` call, so a part will remain `busy` for the entire
time it is being uploaded/retried.
---
 packages/@uppy/aws-s3-multipart/src/MultipartUploader.js | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js b/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
index c67d70975b..9a4973409a 100644
--- a/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
+++ b/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
@@ -278,18 +278,18 @@ class MultipartUploader {
   #uploadPartRetryable (index, prePreparedPart) {
     return this.#retryable({
       before: () => {
+        this.chunkState[index].busy = true
         this.partsInProgress += 1
       },
       attempt: () => this.#uploadPart(index, prePreparedPart),
       after: () => {
+        this.chunkState[index].busy = false
         this.partsInProgress -= 1
       },
     })
   }
 
   #uploadPart (index, prePreparedPart) {
-    this.chunkState[index].busy = true
-
     const valid = typeof prePreparedPart?.url === 'string'
     if (!valid) {
       throw new TypeError('AwsS3/Multipart: Got incorrect result for `prePreparedPart`, expected an object `{ url }`.')
@@ -297,7 +297,6 @@ class MultipartUploader {
 
     const { url, headers } = prePreparedPart
     if (this.#aborted()) {
-      this.chunkState[index].busy = false
       throw createAbortError()
     }
 
@@ -359,14 +358,12 @@ class MultipartUploader {
 
     xhr.addEventListener('abort', () => {
       cleanup()
-      this.chunkState[index].busy = false
 
       defer.reject(createAbortError())
     })
 
     xhr.addEventListener('load', (ev) => {
       cleanup()
-      this.chunkState[index].busy = false
 
       if (ev.target.status < 200 || ev.target.status >= 300) {
         const error = new Error('Non 2xx')
@@ -394,7 +391,6 @@ class MultipartUploader {
 
     xhr.addEventListener('error', (ev) => {
       cleanup()
-      this.chunkState[index].busy = false
 
       const error = new Error('Unknown error')
       error.source = ev.target