@uppy/core: improve performance of validating & uploading files (#4402)

* show how many files are added when loading remake of #4388 * add french (cherry pick) * implement concurrent file listing * refactor / fix lint * refactor/reduce duplication * pull out totals validation don't do it for every file added, as it's very slow instead do the check at the end when all files are added. this allows us to easily work with 10k+ files fixes #4389 * Update packages/@uppy/core/src/Uppy.js Co-authored-by: Antoine du Hamel <[email protected]> * make restricter.validate validate everything instead make more specific methods for sub-validation also rename validateTotals to validateAggregateRestrictions * improve errors and user feedback - handle errors centrally so that we can limit the amount of toasts (informers) sent to the users (prevent flooding hundreds/thousands of them) - introduce FileRestrictionError which is a restriction error for a specific file - introduce isUserFacing field for RestrictionError * fix performance issue reintroduced * improvements - show "%{count} additional restrictions were not fulfilled" for any restriction errors more than 4 - refactor/rename methods - improve ghost logic/comments * improve performance when uploading - introduce new event "upload-start" that can contain multiple files - make a new patchFilesState method to allow updating more files - unify "upload-start" logic in all plugins (send it before files start uploading) - defer slicing buffer until we need the data - refactor to reuse code * fix e2e build issue * try to upgrade cypress maybe it fixes the error * Revert "fix e2e build issue" This reverts commit ff3e580. * upgrade parcel * move mutation logic to end * remove FileRestrictionError merge it with RestrictionError * fix silly bug looks like the e2e tests are doing its job 👏 --------- Co-authored-by: Antoine du Hamel <[email protected]>
transloadit · Apr 15, 2023 · 90f7fb9 · 90f7fb9
1 parent d024eb0
commit 90f7fb9
Show file tree

Hide file tree

Showing 21 changed files with 1,766 additions and 853 deletions.
diff --git a/e2e/package.json b/e2e/package.json
@@ -45,11 +45,11 @@
     "@uppy/zoom": "workspace:^"
   },
   "devDependencies": {
-    "cypress": "^10.0.0",
+    "cypress": "^12.9.0",
     "cypress-terminal-report": "^4.1.2",
     "deep-freeze": "^0.0.1",
     "execa": "^6.1.0",
-    "parcel": "^2.0.1",
+    "parcel": "2.0.0-nightly.1278",
     "prompts": "^2.4.2",
     "react": "^18.1.0",
     "react-dom": "^18.1.0",

diff --git a/package.json b/package.json
@@ -45,7 +45,7 @@
     "@babel/preset-env": "^7.14.7",
     "@babel/register": "^7.10.5",
     "@babel/types": "^7.17.0",
-    "@parcel/transformer-vue": "2.7.0",
+    "@parcel/transformer-vue": "2.8.4-nightly.2903+5b901a317",
     "@types/jasmine": "file:./private/@types/jasmine",
     "@types/jasminewd2": "file:./private/@types/jasmine",
     "@typescript-eslint/eslint-plugin": "^5.0.0",

diff --git a/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js b/packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
@@ -70,19 +70,29 @@ class MultipartUploader {
 
     // Upload zero-sized files in one zero-sized chunk
     if (this.#data.size === 0) {
-      this.#chunks = [this.#data]
-      this.#data.onProgress = this.#onPartProgress(0)
-      this.#data.onComplete = this.#onPartComplete(0)
+      this.#chunks = [{
+        getData: () => this.#data,
+        onProgress: this.#onPartProgress(0),
+        onComplete: this.#onPartComplete(0),
+      }]
     } else {
       const arraySize = Math.ceil(fileSize / chunkSize)
       this.#chunks = Array(arraySize)
-      let j = 0
-      for (let i = 0; i < fileSize; i += chunkSize) {
+
+      for (let i = 0, j = 0; i < fileSize; i += chunkSize, j++) {
         const end = Math.min(fileSize, i + chunkSize)
-        const chunk = this.#data.slice(i, end)
-        chunk.onProgress = this.#onPartProgress(j)
-        chunk.onComplete = this.#onPartComplete(j)
-        this.#chunks[j++] = chunk
+
+        // Defer data fetching/slicing until we actually need the data, because it's slow if we have a lot of files
+        const getData = () => {
+          const i2 = i
+          return this.#data.slice(i2, end)
+        }
+
+        this.#chunks[j] = {
+          getData,
+          onProgress: this.#onPartProgress(j),
+          onComplete: this.#onPartComplete(j),
+        }
       }
     }
 

diff --git a/packages/@uppy/aws-s3-multipart/src/index.js b/packages/@uppy/aws-s3-multipart/src/index.js
@@ -4,7 +4,7 @@ import EventTracker from '@uppy/utils/lib/EventTracker'
 import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress'
 import getSocketHost from '@uppy/utils/lib/getSocketHost'
 import { RateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
-
+import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
 import { createAbortError } from '@uppy/utils/lib/AbortController'
 import packageJson from '../package.json'
 import MultipartUploader from './MultipartUploader.js'
@@ -200,17 +200,23 @@ class HTTPCommunicationQueue {
     return this.#sendCompletionRequest(file, { key, uploadId, parts, signal }).abortOn(signal)
   }
 
-  async uploadChunk (file, partNumber, body, signal) {
+  async uploadChunk (file, partNumber, chunk, signal) {
     throwIfAborted(signal)
     const { uploadId, key } = await this.getUploadId(file, signal)
     throwIfAborted(signal)
     for (;;) {
-      const signature = await this.#fetchSignature(file, { uploadId, key, partNumber, body, signal }).abortOn(signal)
+      const chunkData = chunk.getData()
+      const { onProgress, onComplete } = chunk
+
+      const signature = await this.#fetchSignature(file, {
+        uploadId, key, partNumber, body: chunkData, signal,
+      }).abortOn(signal)
+
       throwIfAborted(signal)
       try {
         return {
           PartNumber: partNumber,
-          ...await this.#uploadPartBytes(signature, body, signal).abortOn(signal),
+          ...await this.#uploadPartBytes({ signature, body: chunkData, onProgress, onComplete, signal }).abortOn(signal),
         }
       } catch (err) {
         if (!await this.#shouldRetry(err)) throw err
@@ -258,8 +264,6 @@ export default class AwsS3Multipart extends BasePlugin {
       }
     }
 
-    this.upload = this.upload.bind(this)
-
     /**
      * Simultaneous upload limiting is shared across all uploads with this plugin.
      *
@@ -369,7 +373,7 @@ export default class AwsS3Multipart extends BasePlugin {
       .then(assertServerError)
   }
 
-  static async uploadPartBytes ({ url, expires, headers }, body, signal) {
+  static async uploadPartBytes ({ signature: { url, expires, headers }, body, onProgress, onComplete, signal }) {
     throwIfAborted(signal)
 
     if (url == null) {
@@ -397,7 +401,7 @@ export default class AwsS3Multipart extends BasePlugin {
       }
       signal.addEventListener('abort', onabort)
 
-      xhr.upload.addEventListener('progress', body.onProgress)
+      xhr.upload.addEventListener('progress', onProgress)
 
       xhr.addEventListener('abort', () => {
         cleanup()
@@ -427,7 +431,7 @@ export default class AwsS3Multipart extends BasePlugin {
           return
         }
 
-        body.onProgress?.(body.size)
+        onProgress?.(body.size)
 
         // NOTE This must be allowed by CORS.
         const etag = ev.target.getResponseHeader('ETag')
@@ -437,7 +441,7 @@ export default class AwsS3Multipart extends BasePlugin {
           return
         }
 
-        body.onComplete?.(etag)
+        onComplete?.(etag)
         resolve({
           ETag: etag,
         })
@@ -466,8 +470,10 @@ export default class AwsS3Multipart extends BasePlugin {
     })
   }
 
-  uploadFile (file) {
+  #uploadFile (file) {
     return new Promise((resolve, reject) => {
+      const getFile = () => this.uppy.getFile(file.id) || file
+
       const onProgress = (bytesUploaded, bytesTotal) => {
         this.uppy.emit('upload-progress', file, {
           uploader: this,
@@ -485,7 +491,6 @@ export default class AwsS3Multipart extends BasePlugin {
       }
 
       const onSuccess = (result) => {
-        const uploadObject = upload // eslint-disable-line no-use-before-define
         const uploadResp = {
           body: {
             ...result,
@@ -495,23 +500,17 @@ export default class AwsS3Multipart extends BasePlugin {
 
         this.resetUploaderReferences(file.id)
 
-        const cFile = this.uppy.getFile(file.id)
-        this.uppy.emit('upload-success', cFile || file, uploadResp)
+        this.uppy.emit('upload-success', getFile(), uploadResp)
 
         if (result.location) {
           this.uppy.log(`Download ${file.name} from ${result.location}`)
         }
 
-        resolve(uploadObject)
+        resolve()
       }
 
       const onPartComplete = (part) => {
-        const cFile = this.uppy.getFile(file.id)
-        if (!cFile) {
-          return
-        }
-
-        this.uppy.emit('s3-multipart:part-uploaded', cFile, part)
+        this.uppy.emit('s3-multipart:part-uploaded', getFile(), part)
       }
 
       const upload = new MultipartUploader(file.data, {
@@ -564,11 +563,7 @@ export default class AwsS3Multipart extends BasePlugin {
         upload.start()
       })
 
-      // Don't double-emit upload-started for Golden Retriever-restored files that were already started
-      if (!file.progress.uploadStarted || !file.isRestored) {
-        upload.start()
-        this.uppy.emit('upload-started', file)
-      }
+      upload.start()
     })
   }
 
@@ -597,14 +592,9 @@ export default class AwsS3Multipart extends BasePlugin {
 
   // NOTE! Keep this duplicated code in sync with other plugins
   // TODO we should probably abstract this into a common function
-  async uploadRemote (file) {
+  async #uploadRemote (file) {
     this.resetUploaderReferences(file.id)
 
-    // Don't double-emit upload-started for Golden Retriever-restored files that were already started
-    if (!file.progress.uploadStarted || !file.isRestored) {
-      this.uppy.emit('upload-started', file)
-    }
-
     try {
       if (file.serverToken) {
         return await this.connectToServerSocket(file)
@@ -733,15 +723,20 @@ export default class AwsS3Multipart extends BasePlugin {
     })
   }
 
-  async upload (fileIDs) {
+  #upload = async (fileIDs) => {
     if (fileIDs.length === 0) return undefined
 
-    const promises = fileIDs.map((id) => {
-      const file = this.uppy.getFile(id)
+    const files = this.uppy.getFilesByIds(fileIDs)
+
+    const filesFiltered = filterNonFailedFiles(files)
+    const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
+    this.uppy.emit('upload-start', filesToEmit)
+
+    const promises = filesFiltered.map((file) => {
       if (file.isRemote) {
-        return this.uploadRemote(file)
+        return this.#uploadRemote(file)
       }
-      return this.uploadFile(file)
+      return this.#uploadFile(file)
     })
 
     return Promise.all(promises)
@@ -810,7 +805,7 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     })
     this.uppy.addPreProcessor(this.#setCompanionHeaders)
-    this.uppy.addUploader(this.upload)
+    this.uppy.addUploader(this.#upload)
   }
 
   uninstall () {
@@ -822,6 +817,6 @@ export default class AwsS3Multipart extends BasePlugin {
       },
     })
     this.uppy.removePreProcessor(this.#setCompanionHeaders)
-    this.uppy.removeUploader(this.upload)
+    this.uppy.removeUploader(this.#upload)
   }
 }
diff --git a/packages/@uppy/aws-s3/src/index.js b/packages/@uppy/aws-s3/src/index.js
@@ -28,6 +28,7 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
 import { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
 import { RequestClient } from '@uppy/companion-client'
+import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
 
 import packageJson from '../package.json'
 import MiniXHRUpload from './MiniXHRUpload.js'
@@ -163,7 +164,7 @@ export default class AwsS3 extends BasePlugin {
       .then(assertServerError)
   }
 
-  #handleUpload = (fileIDs) => {
+  #handleUpload = async (fileIDs) => {
     /**
      * keep track of `getUploadParameters()` responses
      * so we can cancel the calls individually using just a file ID
@@ -178,10 +179,11 @@ export default class AwsS3 extends BasePlugin {
     }
     this.uppy.on('file-removed', onremove)
 
-    fileIDs.forEach((id) => {
-      const file = this.uppy.getFile(id)
-      this.uppy.emit('upload-started', file)
-    })
+    const files = this.uppy.getFilesByIds(fileIDs)
+
+    const filesFiltered = filterNonFailedFiles(files)
+    const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)
+    this.uppy.emit('upload-start', filesToEmit)
 
     const getUploadParameters = this.#requests.wrapPromiseFunction((file) => {
       return this.opts.getUploadParameters(file)

diff --git a/packages/@uppy/core/src/Restricter.js b/packages/@uppy/core/src/Restricter.js
@@ -13,6 +13,12 @@ const defaultOptions = {
 }
 
 class RestrictionError extends Error {
+  constructor (message, { isUserFacing = true, file } = {}) {
+    super(message)
+    this.isUserFacing = isUserFacing
+    if (file != null) this.file = file // only some restriction errors are related to a particular file
+  }
+
   isRestriction = true
 }
 
@@ -30,16 +36,38 @@ class Restricter {
     }
   }
 
-  validate (file, files) {
-    const { maxFileSize, minFileSize, maxTotalFileSize, maxNumberOfFiles, allowedFileTypes } = this.getOpts().restrictions
+  // Because these operations are slow, we cannot run them for every file (if we are adding multiple files)
+  validateAggregateRestrictions (existingFiles, addingFiles) {
+    const { maxTotalFileSize, maxNumberOfFiles } = this.getOpts().restrictions
 
     if (maxNumberOfFiles) {
-      const nonGhostFiles = files.filter(f => !f.isGhost)
-      if (nonGhostFiles.length + 1 > maxNumberOfFiles) {
+      const nonGhostFiles = existingFiles.filter(f => !f.isGhost)
+      if (nonGhostFiles.length + addingFiles.length > maxNumberOfFiles) {
         throw new RestrictionError(`${this.i18n('youCanOnlyUploadX', { smart_count: maxNumberOfFiles })}`)
       }
     }
 
+    if (maxTotalFileSize) {
+      let totalFilesSize = existingFiles.reduce((total, f) => (total + f.size), 0)
+
+      for (const addingFile of addingFiles) {
+        if (addingFile.size != null) { // We can't check maxTotalFileSize if the size is unknown.
+          totalFilesSize += addingFile.size
+
+          if (totalFilesSize > maxTotalFileSize) {
+            throw new RestrictionError(this.i18n('exceedsSize', {
+              size: prettierBytes(maxTotalFileSize),
+              file: addingFile.name,
+            }))
+          }
+        }
+      }
+    }
+  }
+
+  validateSingleFile (file) {
+    const { maxFileSize, minFileSize, allowedFileTypes } = this.getOpts().restrictions
+
     if (allowedFileTypes) {
       const isCorrectFileType = allowedFileTypes.some((type) => {
         // check if this is a mime-type
@@ -57,19 +85,7 @@ class Restricter {
 
       if (!isCorrectFileType) {
         const allowedFileTypesString = allowedFileTypes.join(', ')
-        throw new RestrictionError(this.i18n('youCanOnlyUploadFileTypes', { types: allowedFileTypesString }))
-      }
-    }
-
-    // We can't check maxTotalFileSize if the size is unknown.
-    if (maxTotalFileSize && file.size != null) {
-      const totalFilesSize = files.reduce((total, f) => (total + f.size), file.size)
-
-      if (totalFilesSize > maxTotalFileSize) {
-        throw new RestrictionError(this.i18n('exceedsSize', {
-          size: prettierBytes(maxTotalFileSize),
-          file: file.name,
-        }))
+        throw new RestrictionError(this.i18n('youCanOnlyUploadFileTypes', { types: allowedFileTypesString }), { file })
       }
     }
 
@@ -78,17 +94,24 @@ class Restricter {
       throw new RestrictionError(this.i18n('exceedsSize', {
         size: prettierBytes(maxFileSize),
         file: file.name,
-      }))
+      }), { file })
     }
 
     // We can't check minFileSize if the size is unknown.
     if (minFileSize && file.size != null && file.size < minFileSize) {
       throw new RestrictionError(this.i18n('inferiorSize', {
         size: prettierBytes(minFileSize),
-      }))
+      }), { file })
     }
   }
 
+  validate (existingFiles, addingFiles) {
+    addingFiles.forEach((addingFile) => {
+      this.validateSingleFile(addingFile)
+    })
+    this.validateAggregateRestrictions(existingFiles, addingFiles)
+  }
+
   validateMinNumberOfFiles (files) {
     const { minNumberOfFiles } = this.getOpts().restrictions
     if (Object.keys(files).length < minNumberOfFiles) {