Merge pull request #805 from transloadit/fix/transloadit-failures

More robust failure handling for Transloadit, closes #708

src/core/Core.js

@@ -1107,7 +1107,7 @@ class Uppy {
     // Not returning the `catch`ed promise, because we still want to return a rejected
     // promise from this method if the upload failed.
     lastStep.catch((err) => {
-      this.emit('error', err)
+      this.emit('error', err, uploadID)
 
       this._removeUpload(uploadID)
     })

src/plugins/Transloadit/index.js

@@ -53,6 +53,7 @@ module.exports = class Transloadit extends Plugin {
 
     this.prepareUpload = this.prepareUpload.bind(this)
     this.afterUpload = this.afterUpload.bind(this)
+    this.handleError = this.handleError.bind(this)
     this.onFileUploadURLAvailable = this.onFileUploadURLAvailable.bind(this)
     this.onRestored = this.onRestored.bind(this)
     this.getPersistentData = this.getPersistentData.bind(this)
@@ -227,7 +228,7 @@ module.exports = class Transloadit extends Plugin {
       return this.connectSocket(assembly)
         .then(() => assembly)
     }).then((assembly) => {
-      this.uppy.log('[Transloadit] Created Assembly')
+      this.uppy.log(`[Transloadit] Created Assembly ${assembly.assembly_id}`)
       return assembly
     }).catch((err) => {
       this.uppy.info(this.i18n('creatingAssemblyFailed'), 'error', 0)
@@ -617,7 +618,7 @@ module.exports = class Transloadit extends Plugin {
 
     let optionsPromise
     if (fileIDs.length > 0) {
-      optionsPromise = this.getAssemblyOptions(fileIDs)
+      optionsPromise = Promise.resolve(this.getAssemblyOptions(fileIDs))
         .then((allOptions) => this.dedupeAssemblyOptions(allOptions))
     } else if (this.opts.alwaysRunAssembly) {
       optionsPromise = Promise.resolve(
@@ -634,9 +635,21 @@ module.exports = class Transloadit extends Plugin {
       return Promise.resolve()
     }
 
-    return optionsPromise.then((assemblies) => Promise.all(
-      assemblies.map(createAssembly)
-    ))
+    return optionsPromise.then(
+      (assemblies) => Promise.all(
+        assemblies.map(createAssembly)
+      ),
+      // If something went wrong before any assemblies could be created,
+      // clear all processing state.
+      (err) => {
+        fileIDs.forEach((fileID) => {
+          const file = this.uppy.getFile(fileID)
+          this.uppy.emit('preprocess-complete', file)
+          this.uppy.emit('upload-error', file, err)
+        })
+        throw err
+      }
+    )
   }
 
   afterUpload (fileIDs, uploadID) {
@@ -771,10 +784,27 @@ module.exports = class Transloadit extends Plugin {
     })
   }
 
+  handleError (err, uploadID) {
+    this.uppy.log('[Transloadit] handleError')
+    this.uppy.log(err)
+    this.uppy.log(uploadID)
+    const state = this.getPluginState()
+    const assemblyIDs = state.uploadsAssemblies[uploadID]
+
+    assemblyIDs.forEach((assemblyID) => {
+      if (this.sockets[assemblyID]) {
+        this.sockets[assemblyID].close()
+      }
+    })
+  }
+
   install () {
     this.uppy.addPreProcessor(this.prepareUpload)
     this.uppy.addPostProcessor(this.afterUpload)
 
+    // We may need to close socket.io connections on error.
+    this.uppy.on('error', this.handleError)
+
     if (this.opts.importFromUploadURLs) {
       // No uploader needed when importing; instead we take the upload URL from an existing uploader.
       this.uppy.on('upload-success', this.onFileUploadURLAvailable)
@@ -783,6 +813,9 @@ module.exports = class Transloadit extends Plugin {
         // Disable tus-js-client fingerprinting, otherwise uploading the same file at different times
         // will upload to the same assembly.
         resume: false,
+        // Disable Uppy Server's retry optimisation; we need to change the endpoint on retry
+        // so it can't just reuse the same tus.Upload instance server-side.
+        useFastRemoteRetry: false,
         // Only send assembly metadata to the tus endpoint.
         metaFields: ['assembly_url', 'filename', 'fieldname']
       })
@@ -806,6 +839,7 @@ module.exports = class Transloadit extends Plugin {
   uninstall () {
     this.uppy.removePreProcessor(this.prepareUpload)
     this.uppy.removePostProcessor(this.afterUpload)
+    this.uppy.off('error', this.handleError)
 
     if (this.opts.importFromUploadURLs) {
       this.uppy.off('upload-success', this.onFileUploadURLAvailable)

src/plugins/Transloadit/index.test.js

@@ -165,4 +165,56 @@ describe('Transloadit', () => {
 
     return expect(uppy.upload()).rejects.toEqual(new Error('short-circuited'))
   })
+
+  it('Does not leave lingering progress if getAssemblyOptions fails', () => {
+    const uppy = new Core()
+    uppy.use(Transloadit, {
+      getAssemblyOptions (file) {
+        return Promise.reject(new Error('Failure!'))
+      }
+    })
+
+    uppy.addFile({
+      source: 'jest',
+      name: 'abc',
+      data: new Uint8Array(100)
+    })
+
+    return uppy.upload().then(() => {
+      throw new Error('Should not have succeeded')
+    }, (err) => {
+      const fileID = Object.keys(uppy.getState().files)[0]
+
+      expect(err.message).toBe('Failure!')
+      expect(uppy.getFile(fileID).progress.uploadStarted).toBe(false)
+    })
+  })
+
+  it('Does not leave lingering progress if creating assembly fails', () => {
+    const uppy = new Core()
+    uppy.use(Transloadit, {
+      params: {
+        auth: { key: 'some auth key string' },
+        template_id: 'some template id string'
+      }
+    })
+
+    uppy.getPlugin('Transloadit').client.createAssembly = () =>
+      Promise.reject(new Error('Could not create assembly!'))
+
+    uppy.addFile({
+      source: 'jest',
+      name: 'abc',
+      data: new Uint8Array(100)
+    })
+
+    return uppy.upload().then(() => {
+      throw new Error('Should not have succeeded')
+    }, (err) => {
+      const fileID = Object.keys(uppy.getState().files)[0]
+
+      expect(err.message).toBe('Could not create assembly!')
+      expect(uppy.getFile(fileID).progress.uploadStarted).toBe(false)
+    })
+  })
 })

src/plugins/Tus.js

@@ -60,6 +60,7 @@ module.exports = class Tus extends Plugin {
     const defaultOptions = {
       resume: true,
       autoRetry: true,
+      useFastRemoteRetry: true,
       retryDelays: [0, 1000, 3000, 5000]
     }
 
@@ -304,6 +305,17 @@ module.exports = class Tus extends Plugin {
       socket.on('error', (errData) => {
         const { message } = errData.error
         const error = Object.assign(new Error(message), { cause: errData.error })
+
+        // If the remote retry optimisation should not be used,
+        // close the socket—this will tell uppy-server to clear state and delete the file.
+        if (!this.opts.useFastRemoteRetry) {
+          this.resetUploaderReferences(file.id)
+          // Remove the serverToken so that a new one will be created for the retry.
+          this.uppy.setFileState(file.id, {
+            serverToken: null
+          })
+        }
+
         this.uppy.emit('upload-error', file, error)
         reject(error)
       })