Prechádzať zdrojové kódy

Companion+client stability fixes, error handling and retry (#4734)

* respond with 500 if unhandled upload error

400 seems inappropriate

* add todo

* fix companion download error

respond with correct response code when calling /get
and request to the upstream server fail

also add a unit test for this

* forward 429 from provider

allows us to retry it

* rename fn

* add a way to test refresh tokens

* fix race condtiion with refreshing token

* implement retry and fix socket

- Make sure we don't instantiate a new Provider for every file uploaded (reuse the original provider instead) - this caused the refresh token synchronization not to work
- Retry most errors when uploading (`/get`) using companion, but for HTTP, only retry certain HTTP status codes
- Reimplement the websocket code to be more stable and retry as well

* remove unused socket wrapper

* remove useFastRemoteRetry

because it isn't supported in Companion as far as I can tell

* fix error handling

* remove unneeded logic

* apply review suggestion

* retry awaiting for companion socket too

* retry whole operation instead of individually

or else we risk getting stuck retrying just half of the operation, which might never recover

* improvements

- rewrite so we can use p-retry for exponential backoff for socket reconnect too
- improve logging
- simplify/reuse code

* Update packages/@uppy/utils/src/fetchWithNetworkError.js

Co-authored-by: Antoine du Hamel <antoine@transloadit.com>

* Explain requestClient Symbol with comment

Co-authored-by: Mikael Finstad <finstaden@gmail.com>

* Update packages/@uppy/provider-views/src/View.js

* fixes

preventing socket.send when the socket is not in the connected state
catch errors in handlers

* log if trying to refresh with no token

* log whether we got a refresh token

* dont log retrying when in reality not

* always prompt: consent

for google drive

* add signal to post

* refactor away eventmanager

and event handler error catching

* don't fail with upload-error

if a file is canceled

* fix expect

* add capabilities support

* make requestClient non-enumerable

---------

Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
Co-authored-by: Merlijn Vos <merlijn@soverin.net>
Mikael Finstad 1 rok pred
rodič
commit
17826da517
31 zmenil súbory, kde vykonal 477 pridanie a 490 odobranie
  1. 3 7
      packages/@uppy/aws-s3-multipart/src/index.js
  2. 3 7
      packages/@uppy/aws-s3/src/index.js
  3. 2 1
      packages/@uppy/companion-client/package.json
  4. 9 7
      packages/@uppy/companion-client/src/Provider.js
  5. 276 142
      packages/@uppy/companion-client/src/RequestClient.js
  6. 0 87
      packages/@uppy/companion-client/src/Socket.js
  7. 0 176
      packages/@uppy/companion-client/src/Socket.test.js
  8. 0 1
      packages/@uppy/companion-client/src/index.js
  9. 6 1
      packages/@uppy/companion/src/config/grant.js
  10. 5 3
      packages/@uppy/companion/src/server/controllers/callback.js
  11. 4 4
      packages/@uppy/companion/src/server/controllers/get.js
  12. 2 1
      packages/@uppy/companion/src/server/controllers/refresh-token.js
  13. 3 3
      packages/@uppy/companion/src/server/controllers/url.js
  14. 2 2
      packages/@uppy/companion/src/server/helpers/upload.js
  15. 27 8
      packages/@uppy/companion/src/server/helpers/utils.js
  16. 19 0
      packages/@uppy/companion/src/server/provider/drive/index.js
  17. 4 0
      packages/@uppy/companion/src/server/provider/error.js
  18. 30 9
      packages/@uppy/companion/src/server/provider/providerErrors.js
  19. 7 7
      packages/@uppy/companion/src/server/socket.js
  20. 30 0
      packages/@uppy/companion/test/__tests__/companion.js
  21. 1 0
      packages/@uppy/companion/test/__tests__/provider-manager.js
  22. 3 1
      packages/@uppy/companion/test/fixtures/drive.js
  23. 6 1
      packages/@uppy/provider-views/src/View.js
  24. 0 3
      packages/@uppy/transloadit/src/index.js
  25. 2 8
      packages/@uppy/tus/src/index.js
  26. 0 1
      packages/@uppy/tus/types/index.d.ts
  27. 3 1
      packages/@uppy/url/src/Url.jsx
  28. 1 1
      packages/@uppy/utils/src/RateLimitedQueue.js
  29. 1 1
      packages/@uppy/utils/src/fetchWithNetworkError.js
  30. 2 7
      packages/@uppy/xhr-upload/src/index.js
  31. 26 0
      yarn.lock

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

@@ -1,5 +1,5 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
-import { Provider, RequestClient } from '@uppy/companion-client'
+import { RequestClient } from '@uppy/companion-client'
 import EventManager from '@uppy/utils/lib/EventManager'
 import { RateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
 import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
@@ -869,11 +869,7 @@ export default class AwsS3Multipart extends BasePlugin {
 
     const promises = filesFiltered.map((file) => {
       if (file.isRemote) {
-        // INFO: the url plugin needs to use RequestClient,
-        // while others use Provider
-        const Client = file.remote.providerOptions.provider ? Provider : RequestClient
         const getQueue = () => this.requests
-        const client = new Client(this.uppy, file.remote.providerOptions, getQueue)
         this.#setResumableUploadsCapability(false)
         const controller = new AbortController()
 
@@ -882,10 +878,10 @@ export default class AwsS3Multipart extends BasePlugin {
         }
         this.uppy.on('file-removed', removedHandler)
 
-        const uploadPromise = client.uploadRemoteFile(
+        const uploadPromise = file.remote.requestClient.uploadRemoteFile(
           file,
           this.#getCompanionClientArgs(file),
-          { signal: controller.signal },
+          { signal: controller.signal, getQueue },
         )
 
         this.requests.wrapSyncFunction(() => {

+ 3 - 7
packages/@uppy/aws-s3/src/index.js

@@ -28,7 +28,7 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
 import AwsS3Multipart from '@uppy/aws-s3-multipart'
 import { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
-import { RequestClient, Provider } from '@uppy/companion-client'
+import { RequestClient } from '@uppy/companion-client'
 import { filterNonFailedFiles, filterFilesToEmitUploadStarted } from '@uppy/utils/lib/fileFilters'
 
 import packageJson from '../package.json'
@@ -273,11 +273,7 @@ export default class AwsS3 extends BasePlugin {
     if (file.error) throw new Error(file.error)
 
     if (file.isRemote) {
-      // INFO: the url plugin needs to use RequestClient,
-      // while others use Provider
-      const Client = file.remote.providerOptions.provider ? Provider : RequestClient
       const getQueue = () => this.#requests
-      const client = new Client(this.uppy, file.remote.providerOptions, getQueue)
       const controller = new AbortController()
 
       const removedHandler = (removedFile) => {
@@ -285,10 +281,10 @@ export default class AwsS3 extends BasePlugin {
       }
       this.uppy.on('file-removed', removedHandler)
 
-      const uploadPromise = client.uploadRemoteFile(
+      const uploadPromise = file.remote.requestClient.uploadRemoteFile(
         file,
         this.#getCompanionClientArgs(file),
-        { signal: controller.signal },
+        { signal: controller.signal, getQueue },
       )
 
       this.#requests.wrapSyncFunction(() => {

+ 2 - 1
packages/@uppy/companion-client/package.json

@@ -23,7 +23,8 @@
   },
   "dependencies": {
     "@uppy/utils": "workspace:^",
-    "namespace-emitter": "^2.0.1"
+    "namespace-emitter": "^2.0.1",
+    "p-retry": "^6.1.0"
   },
   "devDependencies": {
     "vitest": "^0.34.5"

+ 9 - 7
packages/@uppy/companion-client/src/Provider.js

@@ -30,8 +30,8 @@ function isOriginAllowed (origin, allowedOrigin) {
 export default class Provider extends RequestClient {
   #refreshingTokenPromise
 
-  constructor (uppy, opts, getQueue) {
-    super(uppy, opts, getQueue)
+  constructor (uppy, opts) {
+    super(uppy, opts)
     this.provider = opts.provider
     this.id = this.provider
     this.name = this.opts.name || getName(this.id)
@@ -140,8 +140,7 @@ export default class Provider extends RequestClient {
 
         authWindow.close()
         window.removeEventListener('message', handleToken)
-        this.setAuthToken(data.token)
-        resolve()
+        this.setAuthToken(data.token).then(() => resolve()).catch(reject)
       }
       window.addEventListener('message', handleToken)
     })
@@ -160,21 +159,24 @@ export default class Provider extends RequestClient {
     await this.#refreshingTokenPromise
 
     try {
-      // throw Object.assign(new Error(), { isAuthError: true }) // testing simulate access token expired (to refresh token)
-      // A better way to test this is for example with Google Drive:
+      // to test simulate access token expired (leading to a token token refresh),
+      // see mockAccessTokenExpiredError in companion/drive.
+      // If you want to test refresh token *and* access token invalid, do this for example with Google Drive:
       // While uploading, go to your google account settings,
       // "Third-party apps & services", then click "Companion" and "Remove access".
 
       return await super.request(...args)
     } catch (err) {
       // only handle auth errors (401 from provider), and only handle them if we have a (refresh) token
-      if (!err.isAuthError || !(await this.#getAuthToken())) throw err
+      const authTokenAfter = await this.#getAuthToken()
+      if (!err.isAuthError || !authTokenAfter) throw err
 
       if (this.#refreshingTokenPromise == null) {
         // Many provider requests may be starting at once, however refresh token should only be called once.
         // Once a refresh token operation has started, we need all other request to wait for this operation (atomically)
         this.#refreshingTokenPromise = (async () => {
           try {
+            this.uppy.log(`[CompanionClient] Refreshing expired auth token`, 'info')
             const response = await super.request({ path: this.refreshTokenUrl(), method: 'POST' })
             await this.setAuthToken(response.uppyAuthToken)
           } catch (refreshTokenErr) {

+ 276 - 142
packages/@uppy/companion-client/src/RequestClient.js

@@ -1,13 +1,14 @@
 'use strict'
 
+// eslint-disable-next-line import/no-extraneous-dependencies
+import pRetry, { AbortError } from 'p-retry'
+
 import fetchWithNetworkError from '@uppy/utils/lib/fetchWithNetworkError'
 import ErrorWithCause from '@uppy/utils/lib/ErrorWithCause'
 import emitSocketProgress from '@uppy/utils/lib/emitSocketProgress'
 import getSocketHost from '@uppy/utils/lib/getSocketHost'
-import EventManager from '@uppy/utils/lib/EventManager'
 
 import AuthError from './AuthError.js'
-import Socket from './Socket.js'
 
 import packageJson from '../package.json'
 
@@ -16,19 +17,33 @@ function stripSlash (url) {
   return url.replace(/\/$/, '')
 }
 
+const retryCount = 10 // set to a low number, like 2 to test manual user retries
+const socketActivityTimeoutMs = 5 * 60 * 1000 // set to a low number like 10000 to test this
+
+const authErrorStatusCode = 401
+
+class HttpError extends Error {
+  statusCode
+
+  constructor({ statusCode, message }) {
+    super(message)
+    this.statusCode = statusCode
+  }
+}
+
 async function handleJSONResponse (res) {
-  if (res.status === 401) {
+  if (res.status === authErrorStatusCode) {
     throw new AuthError()
   }
 
-  const jsonPromise = res.json()
   if (res.ok) {
-    return jsonPromise
+    return res.json()
   }
 
   let errMsg = `Failed request with status: ${res.status}. ${res.statusText}`
   try {
-    const errData = await jsonPromise
+    const errData = await res.json()
+
     errMsg = errData.message ? `${errMsg} message: ${errData.message}` : errMsg
     errMsg = errData.requestId
       ? `${errMsg} request-Id: ${errData.requestId}`
@@ -36,7 +51,8 @@ async function handleJSONResponse (res) {
   } catch {
     /* if the response contains invalid JSON, let's ignore the error */
   }
-  throw new Error(errMsg)
+
+  throw new HttpError({ statusCode: res.status, message: errMsg })
 }
 
 // todo pull out into core instead?
@@ -47,10 +63,9 @@ export default class RequestClient {
 
   #companionHeaders
 
-  constructor (uppy, opts, getQueue) {
+  constructor (uppy, opts) {
     this.uppy = uppy
     this.opts = opts
-    this.getQueue = getQueue
     this.onReceiveResponse = this.onReceiveResponse.bind(this)
     this.#companionHeaders = opts?.companionHeaders
   }
@@ -192,9 +207,12 @@ export default class RequestClient {
         body: data ? JSON.stringify(data) : null,
       })
       if (!skipPostResponse) this.onReceiveResponse(response)
-      return handleJSONResponse(response)
+
+      return await handleJSONResponse(response)
     } catch (err) {
-      if (err?.isAuthError) throw err
+      // pass these through
+      if (err instanceof AuthError || err.name === 'AbortError') throw err
+
       throw new ErrorWithCause(`Could not ${method} ${this.#getUrl(path)}`, {
         cause: err,
       })
@@ -222,39 +240,79 @@ export default class RequestClient {
     return this.request({ ...options, path, method: 'DELETE', data })
   }
 
+  /**
+   * Remote uploading consists of two steps:
+   * 1. #requestSocketToken which starts the download/upload in companion and returns a unique token for the upload.
+   * Then companion will halt the upload until:
+   * 2. #awaitRemoteFileUpload is called, which will open/ensure a websocket connection towards companion, with the
+   * previously generated token provided. It returns a promise that will resolve/reject once the file has finished
+   * uploading or is otherwise done (failed, canceled)
+   * 
+   * @param {*} file 
+   * @param {*} reqBody 
+   * @param {*} options 
+   * @returns 
+   */
   async uploadRemoteFile (file, reqBody, options = {}) {
     try {
-      if (file.serverToken) {
-        return await this.connectToServerSocket(file, this.getQueue())
-      }
-      const queueRequestSocketToken = this.getQueue().wrapPromiseFunction(
-        this.#requestSocketToken,
-        { priority: -1 },
-      )
-      const serverToken = await queueRequestSocketToken(file, reqBody).abortOn(
-        options.signal,
-      )
-
-      if (!this.uppy.getState().files[file.id]) return undefined
-
-      this.uppy.setFileState(file.id, { serverToken })
-      return await this.connectToServerSocket(
-        this.uppy.getFile(file.id),
-        this.getQueue(),
-      )
+      const { signal, getQueue } = options
+
+      return await pRetry(async () => {
+        // if we already have a serverToken, assume that we are resuming the existing server upload id
+        const existingServerToken = this.uppy.getFile(file.id)?.serverToken;
+        if (existingServerToken != null) {
+          this.uppy.log(`Connecting to exiting websocket ${existingServerToken}`)
+          return this.#awaitRemoteFileUpload({ file, queue: getQueue(), signal })
+        }
+
+        const queueRequestSocketToken = getQueue().wrapPromiseFunction(async (...args) => {
+          try {
+            return await this.#requestSocketToken(...args)
+          } catch (outerErr) {
+            // throwing AbortError will cause p-retry to stop retrying
+            if (outerErr instanceof AuthError) throw new AbortError(outerErr)
+
+            if (outerErr.cause == null) throw outerErr
+            const err = outerErr.cause
+
+            const isRetryableHttpError = () => (
+              [408, 409, 429, 418, 423].includes(err.statusCode)
+              || (err.statusCode >= 500 && err.statusCode <= 599 && ![501, 505].includes(err.statusCode))
+            )
+            if (err instanceof HttpError && !isRetryableHttpError()) throw new AbortError(err);
+  
+            // p-retry will retry most other errors,
+            // but it will not retry TypeError (except network error TypeErrors)
+            throw err
+          }
+        }, { priority: -1 })
+
+        const serverToken = await queueRequestSocketToken({ file, postBody: reqBody, signal }).abortOn(signal)
+
+        if (!this.uppy.getFile(file.id)) return undefined // has file since been removed?
+
+        this.uppy.setFileState(file.id, { serverToken })
+
+        return this.#awaitRemoteFileUpload({
+          file: this.uppy.getFile(file.id), // re-fetching file because it might have changed in the meantime
+          queue: getQueue(),
+          signal
+        })
+      }, { retries: retryCount, signal, onFailedAttempt: (err) => this.uppy.log(`Retrying upload due to: ${err.message}`, 'warning') });
     } catch (err) {
-      if (err?.cause?.name === 'AbortError') {
+      // this is a bit confusing, but note that an error with the `name` prop set to 'AbortError' (from AbortController)
+      // is not the same as `p-retry` `AbortError`
+      if (err.name === 'AbortError') {
         // The file upload was aborted, it’s not an error
         return undefined
       }
 
-      this.uppy.setFileState(file.id, { serverToken: undefined })
       this.uppy.emit('upload-error', file, err)
       throw err
     }
   }
 
-  #requestSocketToken = async (file, postBody) => {
+  #requestSocketToken = async ({ file, postBody, signal }) => {
     if (file.remote.url == null) {
       throw new Error('Cannot connect to an undefined URL')
     }
@@ -262,139 +320,215 @@ export default class RequestClient {
     const res = await this.post(file.remote.url, {
       ...file.remote.body,
       ...postBody,
-    })
+    }, signal)
 
     return res.token
   }
 
   /**
-   * @param {UppyFile} file
+   * This method will ensure a websocket for the specified file and returns a promise that resolves
+   * when the file has finished downloading, or rejects if it fails.
+   * It will retry if the websocket gets disconnected
+   * 
+   * @param {{ file: UppyFile, queue: RateLimitedQueue, signal: AbortSignal }} file
    */
-  async connectToServerSocket (file, queue) {
-    return new Promise((resolve, reject) => {
-      const token = file.serverToken
-      const host = getSocketHost(file.remote.companionUrl)
-      const socket = new Socket({
-        target: `${host}/api/${token}`,
-        autoOpen: false,
-      })
-      const eventManager = new EventManager(this.uppy)
+  async #awaitRemoteFileUpload ({ file, queue, signal }) {
+    let removeEventHandlers
 
-      let queuedRequest
-
-      eventManager.onFileRemove(file.id, () => {
-        socket.send('cancel', {})
-        queuedRequest.abort()
-        resolve(`upload ${file.id} was removed`)
-      })
+    const { capabilities } = this.uppy.getState()
 
-      eventManager.onPause(file.id, (isPaused) => {
-        if (isPaused) {
-          // Remove this file from the queue so another file can start in its place.
-          socket.send('pause', {})
-          queuedRequest.abort()
-        } else {
-          // Resuming an upload should be queued, else you could pause and then
-          // resume a queued upload to make it skip the queue.
-          queuedRequest.abort()
-          queuedRequest = queue.run(() => {
-            socket.open()
-            socket.send('resume', {})
-
-            return () => {}
-          })
+    try {
+      return await new Promise((resolve, reject) => {
+        const token = file.serverToken
+        const host = getSocketHost(file.remote.companionUrl)
+
+        /** @type {WebSocket} */
+        let socket
+        /** @type {AbortController?} */
+        let socketAbortController
+        let activityTimeout
+
+        let { isPaused } = file
+
+        const socketSend = (action, payload) => {
+          if (socket == null || socket.readyState !== socket.OPEN) {
+            this.uppy.log(`Cannot send "${action}" to socket ${file.id} because the socket state was ${String(socket?.readyState)}`, 'warning')
+            return
+          }
+
+          socket.send(JSON.stringify({
+            action,
+            payload: payload ?? {},
+          }))    
+        };
+
+        function sendState() {
+          if (!capabilities.resumableUploads) return;
+
+          if (isPaused) socketSend('pause')
+          else socketSend('resume')
         }
-      })
 
-      eventManager.onPauseAll(file.id, () => {
-        socket.send('pause', {})
-        queuedRequest.abort()
-      })
-
-      eventManager.onCancelAll(file.id, ({ reason } = {}) => {
-        if (reason === 'user') {
-          socket.send('cancel', {})
-          queuedRequest.abort()
+        const createWebsocket = async () => {
+          if (socketAbortController) socketAbortController.abort()
+          socketAbortController = new AbortController()
+
+          const onFatalError = (err) => {
+            // Remove the serverToken so that a new one will be created for the retry.
+            this.uppy.setFileState(file.id, { serverToken: null })
+            socketAbortController?.abort?.()
+            reject(err)
+          }
+  
+          // todo instead implement the ability for users to cancel / retry *currently uploading files* in the UI
+          function resetActivityTimeout() {
+            clearTimeout(activityTimeout)
+            if (isPaused) return
+            activityTimeout = setTimeout(() => onFatalError(new Error('Timeout waiting for message from Companion socket')), socketActivityTimeoutMs)
+          }
+
+          try {
+            await queue.wrapPromiseFunction(async () => {
+              // eslint-disable-next-line promise/param-names
+              const reconnectWebsocket = async () => new Promise((resolveSocket, rejectSocket) => {
+                socket = new WebSocket(`${host}/api/${token}`)
+
+                resetActivityTimeout()
+
+                socket.addEventListener('close', () => {
+                  socket = undefined
+                  rejectSocket(new Error('Socket closed unexpectedly'))
+                })
+
+                socket.addEventListener('error', (error) => {
+                  this.uppy.log(`Companion socket error ${JSON.stringify(error)}, closing socket`, 'warning')
+                  socket.close() // will 'close' event to be emitted
+                })
+
+                socket.addEventListener('open', () => {
+                  sendState()
+                })
+
+                socket.addEventListener('message', (e) => {
+                  resetActivityTimeout()
+
+                  try {
+                    const { action, payload } = JSON.parse(e.data)
+            
+                    switch (action) {
+                      case 'progress': {
+                        emitSocketProgress(this, payload, file)
+                        break;
+                      }
+                      case 'success': {
+                        this.uppy.emit('upload-success', file, { uploadURL: payload.url })
+                        socketAbortController?.abort?.()
+                        resolve()
+                        break;
+                      }
+                      case 'error': {
+                        const { message } = payload.error
+                        throw Object.assign(new Error(message), { cause: payload.error })
+                      }
+                        default:
+                          this.uppy.log(`Companion socket unknown action ${action}`, 'warning')
+                    }
+                  } catch (err) {
+                    onFatalError(err)
+                  }
+                })
+
+                const closeSocket = () => {
+                  this.uppy.log(`Closing socket ${file.id}`, 'info')
+                  clearTimeout(activityTimeout)
+                  if (socket) socket.close()
+                  socket = undefined
+                }
+        
+                socketAbortController.signal.addEventListener('abort', () => {
+                  closeSocket()
+                })
+              })
+
+              await pRetry(reconnectWebsocket, {
+                retries: retryCount,
+                signal: socketAbortController.signal,
+                onFailedAttempt: () => {
+                  if (socketAbortController.signal.aborted) return // don't log in this case
+                  this.uppy.log(`Retrying websocket ${file.id}`, 'info')
+                },
+              });
+            })().abortOn(socketAbortController.signal);
+          } catch (err) {
+            if (socketAbortController.signal.aborted) return
+            onFatalError(err)
+          }
         }
-        resolve(`upload ${file.id} was canceled`)
-      })
 
-      eventManager.onResumeAll(file.id, () => {
-        queuedRequest.abort()
-        if (file.error) {
-          socket.send('pause', {})
-        }
-        queuedRequest = queue.run(() => {
-          socket.open()
-          socket.send('resume', {})
+        const pause = (newPausedState) => {
+          if (!capabilities.resumableUploads) return;
 
-          return () => {}
-        })
-      })
+          isPaused = newPausedState
+          if (socket) sendState()
 
-      eventManager.onRetry(file.id, () => {
-        // Only do the retry if the upload is actually in progress;
-        // else we could try to send these messages when the upload is still queued.
-        // We may need a better check for this since the socket may also be closed
-        // for other reasons, like network failures.
-        if (socket.isOpen) {
-          socket.send('pause', {})
-          socket.send('resume', {})
+          if (newPausedState) {
+            // Remove this file from the queue so another file can start in its place.
+            socketAbortController?.abort?.() // close socket to free up the request for other uploads
+          } else {
+            // Resuming an upload should be queued, else you could pause and then
+            // resume a queued upload to make it skip the queue.
+            createWebsocket()
+          }
         }
-      })
 
-      eventManager.onRetryAll(file.id, () => {
-        // See the comment in the onRetry() call
-        if (socket.isOpen) {
-          socket.send('pause', {})
-          socket.send('resume', {})
+        const onFileRemove = (targetFile) => {
+          if (!capabilities.individualCancellation) return
+          if (targetFile.id !== file.id) return
+          socketSend('cancel')
+          socketAbortController?.abort?.()
+          this.uppy.log(`upload ${file.id} was removed`, 'info')
+          resolve()
         }
-      })
 
-      socket.on('progress', (progressData) => emitSocketProgress(this, progressData, file))
-
-      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 companion to clear state and delete the file.
-        if (!this.opts.useFastRemoteRetry) {
-          // Remove the serverToken so that a new one will be created for the retry.
-          this.uppy.setFileState(file.id, {
-            serverToken: null,
-          })
-        } else {
-          socket.close()
+        const onCancelAll = ({ reason }) => {
+          if (reason === 'user') {
+            socketSend('cancel')
+          }
+          socketAbortController?.abort?.()
+          this.uppy.log(`upload ${file.id} was canceled`, 'info')
+          resolve()
+        };
+
+        const onFilePausedChange = (targetFileId, newPausedState) => {
+          if (targetFileId !== file.id) return
+          pause(newPausedState)
         }
 
-        this.uppy.emit('upload-error', file, error)
-        queuedRequest.done()
-        reject(error)
-      })
-
-      socket.on('success', (data) => {
-        const uploadResp = {
-          uploadURL: data.url,
+        const onPauseAll = () => pause(true)
+        const onResumeAll = () => pause(false)
+
+        this.uppy.on('file-removed', onFileRemove)
+        this.uppy.on('cancel-all', onCancelAll)
+        this.uppy.on('upload-pause', onFilePausedChange)
+        this.uppy.on('pause-all', onPauseAll)
+        this.uppy.on('resume-all', onResumeAll)
+
+        removeEventHandlers = () => {
+          this.uppy.off('file-removed', onFileRemove)
+          this.uppy.off('cancel-all', onCancelAll)
+          this.uppy.off('upload-pause', onFilePausedChange)
+          this.uppy.off('pause-all', onPauseAll)
+          this.uppy.off('resume-all', onResumeAll)
         }
 
-        this.uppy.emit('upload-success', file, uploadResp)
-        queuedRequest.done()
-        socket.close()
-        resolve()
-      })
-
-      queuedRequest = queue.run(() => {
-        if (file.isPaused) {
-          socket.send('pause', {})
-        } else {
-          socket.open()
-        }
+        signal.addEventListener('abort', () => {
+          socketAbortController?.abort();
+        })
 
-        return () => {}
+        createWebsocket()
       })
-    })
+    } finally {
+      removeEventHandlers?.()
+    }
   }
 }

+ 0 - 87
packages/@uppy/companion-client/src/Socket.js

@@ -1,87 +0,0 @@
-import ee from 'namespace-emitter'
-
-export default class UppySocket {
-  #queued = []
-
-  #emitter = ee()
-
-  #isOpen = false
-
-  #socket
-
-  constructor (opts) {
-    this.opts = opts
-
-    if (!opts || opts.autoOpen !== false) {
-      this.open()
-    }
-  }
-
-  get isOpen () { return this.#isOpen }
-
-  [Symbol.for('uppy test: getSocket')] () { return this.#socket }
-
-  [Symbol.for('uppy test: getQueued')] () { return this.#queued }
-
-  open () {
-    if (this.#socket != null) return
-
-    this.#socket = new WebSocket(this.opts.target)
-
-    this.#socket.onopen = () => {
-      this.#isOpen = true
-
-      while (this.#queued.length > 0 && this.#isOpen) {
-        const first = this.#queued.shift()
-        this.send(first.action, first.payload)
-      }
-    }
-
-    this.#socket.onclose = () => {
-      this.#isOpen = false
-      this.#socket = null
-    }
-
-    this.#socket.onmessage = this.#handleMessage
-  }
-
-  close () {
-    this.#socket?.close()
-  }
-
-  send (action, payload) {
-    // attach uuid
-
-    if (!this.#isOpen) {
-      this.#queued.push({ action, payload })
-      return
-    }
-
-    this.#socket.send(JSON.stringify({
-      action,
-      payload,
-    }))
-  }
-
-  on (action, handler) {
-    this.#emitter.on(action, handler)
-  }
-
-  emit (action, payload) {
-    this.#emitter.emit(action, payload)
-  }
-
-  once (action, handler) {
-    this.#emitter.once(action, handler)
-  }
-
-  #handleMessage = (e) => {
-    try {
-      const message = JSON.parse(e.data)
-      this.emit(message.action, message.payload)
-    } catch (err) {
-      // TODO: use a more robust error handler.
-      console.log(err) // eslint-disable-line no-console
-    }
-  }
-}

+ 0 - 176
packages/@uppy/companion-client/src/Socket.test.js

@@ -1,176 +0,0 @@
-import { afterEach, beforeEach, vi, describe, it, expect } from 'vitest'
-import UppySocket from './Socket.js'
-
-describe('Socket', () => {
-  let webSocketConstructorSpy
-  let webSocketCloseSpy
-  let webSocketSendSpy
-
-  beforeEach(() => {
-    webSocketConstructorSpy = vi.fn()
-    webSocketCloseSpy = vi.fn()
-    webSocketSendSpy = vi.fn()
-
-    globalThis.WebSocket = class WebSocket {
-      constructor (target) {
-        webSocketConstructorSpy(target)
-      }
-
-      // eslint-disable-next-line class-methods-use-this
-      close (args) {
-        webSocketCloseSpy(args)
-      }
-
-      // eslint-disable-next-line class-methods-use-this
-      send (json) {
-        webSocketSendSpy(json)
-      }
-
-      triggerOpen () {
-        this.onopen()
-      }
-
-      triggerClose () {
-        this.onclose()
-      }
-    }
-  })
-  afterEach(() => {
-    globalThis.WebSocket = undefined
-  })
-
-  it('should expose a class', () => {
-    expect(UppySocket.name).toEqual('UppySocket')
-    expect(
-      new UppySocket({
-        target: 'foo',
-      }) instanceof UppySocket,
-    )
-  })
-
-  it('should setup a new WebSocket', () => {
-    new UppySocket({ target: 'foo' }) // eslint-disable-line no-new
-    expect(webSocketConstructorSpy.mock.calls[0][0]).toEqual('foo')
-  })
-
-  it('should send a message via the websocket if the connection is open', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
-    webSocketInstance.triggerOpen()
-
-    uppySocket.send('bar', 'boo')
-    expect(webSocketSendSpy.mock.calls.length).toEqual(1)
-    expect(webSocketSendSpy.mock.calls[0]).toEqual([
-      JSON.stringify({ action: 'bar', payload: 'boo' }),
-    ])
-  })
-
-  it('should queue the message for the websocket if the connection is not open', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-
-    uppySocket.send('bar', 'boo')
-    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([{ action: 'bar', payload: 'boo' }])
-    expect(webSocketSendSpy.mock.calls.length).toEqual(0)
-  })
-
-  it('should queue any messages for the websocket if the connection is not open, then send them when the connection is open', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
-
-    uppySocket.send('bar', 'boo')
-    uppySocket.send('moo', 'baa')
-    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([
-      { action: 'bar', payload: 'boo' },
-      { action: 'moo', payload: 'baa' },
-    ])
-    expect(webSocketSendSpy.mock.calls.length).toEqual(0)
-
-    webSocketInstance.triggerOpen()
-
-    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([])
-    expect(webSocketSendSpy.mock.calls.length).toEqual(2)
-    expect(webSocketSendSpy.mock.calls[0]).toEqual([
-      JSON.stringify({ action: 'bar', payload: 'boo' }),
-    ])
-    expect(webSocketSendSpy.mock.calls[1]).toEqual([
-      JSON.stringify({ action: 'moo', payload: 'baa' }),
-    ])
-  })
-
-  it('should start queuing any messages when the websocket connection is closed', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
-    webSocketInstance.triggerOpen()
-    uppySocket.send('bar', 'boo')
-    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([])
-
-    webSocketInstance.triggerClose()
-    uppySocket.send('bar', 'boo')
-    expect(uppySocket[Symbol.for('uppy test: getQueued')]()).toEqual([{ action: 'bar', payload: 'boo' }])
-  })
-
-  it('should close the websocket when it is force closed', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
-    webSocketInstance.triggerOpen()
-
-    uppySocket.close()
-    expect(webSocketCloseSpy.mock.calls.length).toEqual(1)
-  })
-
-  it('should be able to subscribe to messages received on the websocket', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-    const webSocketInstance = uppySocket[Symbol.for('uppy test: getSocket')]()
-
-    const emitterListenerMock = vi.fn()
-    uppySocket.on('hi', emitterListenerMock)
-
-    webSocketInstance.triggerOpen()
-    webSocketInstance.onmessage({
-      data: JSON.stringify({ action: 'hi', payload: 'ho' }),
-    })
-    expect(emitterListenerMock.mock.calls).toEqual([
-      ['ho', undefined, undefined, undefined, undefined, undefined],
-    ])
-  })
-
-  it('should be able to emit messages and subscribe to them', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-
-    const emitterListenerMock = vi.fn()
-    uppySocket.on('hi', emitterListenerMock)
-
-    uppySocket.emit('hi', 'ho')
-    uppySocket.emit('hi', 'ho')
-    uppySocket.emit('hi', 'off to work we go')
-
-    expect(emitterListenerMock.mock.calls).toEqual([
-      ['ho', undefined, undefined, undefined, undefined, undefined],
-      ['ho', undefined, undefined, undefined, undefined, undefined],
-      [
-        'off to work we go',
-        undefined,
-        undefined,
-        undefined,
-        undefined,
-        undefined,
-      ],
-    ])
-  })
-
-  it('should be able to subscribe to the first event for a particular action', () => {
-    const uppySocket = new UppySocket({ target: 'foo' })
-
-    const emitterListenerMock = vi.fn()
-    uppySocket.once('hi', emitterListenerMock)
-
-    uppySocket.emit('hi', 'ho')
-    uppySocket.emit('hi', 'ho')
-    uppySocket.emit('hi', 'off to work we go')
-
-    expect(emitterListenerMock.mock.calls.length).toEqual(1)
-    expect(emitterListenerMock.mock.calls).toEqual([
-      ['ho', undefined, undefined, undefined, undefined, undefined],
-    ])
-  })
-})

+ 0 - 1
packages/@uppy/companion-client/src/index.js

@@ -7,4 +7,3 @@
 export { default as RequestClient } from './RequestClient.js'
 export { default as Provider } from './Provider.js'
 export { default as SearchProvider } from './SearchProvider.js'
-export { default as Socket } from './Socket.js'

+ 6 - 1
packages/@uppy/companion/src/config/grant.js

@@ -8,7 +8,12 @@ module.exports = () => {
         'https://www.googleapis.com/auth/drive.readonly',
       ],
       callback: '/drive/callback',
-      custom_params: { access_type : 'offline' },
+      // access_type: offline is needed in order to get refresh tokens.
+      // prompt: 'consent' is needed because sometimes a user will get stuck in an authenticated state where we will
+      // receive no refresh tokens from them. This seems to be happen when running on different subdomains.
+      // therefore to be safe that we always get refresh tokens, we set this.
+      // https://stackoverflow.com/questions/10827920/not-receiving-google-oauth-refresh-token/65108513#65108513
+      custom_params: { access_type : 'offline', prompt: 'consent' },
     },
     dropbox: {
       transport: 'session',

+ 5 - 3
packages/@uppy/companion/src/server/controllers/callback.js

@@ -41,12 +41,14 @@ module.exports = function callback (req, res, next) { // eslint-disable-line no-
     return res.status(400).send(closePageHtml(origin))
   }
 
+  const { access_token: accessToken, refresh_token: refreshToken } = grant.response
+
   if (!req.companion.allProvidersTokens) req.companion.allProvidersTokens = {}
   req.companion.allProvidersTokens[providerName] = {
-    accessToken: grant.response.access_token,
-    refreshToken: grant.response.refresh_token, // might be undefined for some providers
+    accessToken,
+    refreshToken, // might be undefined for some providers
   }
-  logger.debug(`Generating auth token for provider ${providerName}`, null, req.id)
+  logger.debug(`Generating auth token for provider ${providerName}. refreshToken: ${refreshToken ? 'yes' : 'no'}`, null, req.id)
   const uppyAuthToken = tokenService.generateEncryptedAuthToken(
     req.companion.allProvidersTokens, req.companion.options.secret,
   )

+ 4 - 4
packages/@uppy/companion/src/server/controllers/get.js

@@ -15,12 +15,12 @@ async function get (req, res) {
     return stream
   }
 
-  function onUnhandledError (err) {
+  try {
+    await startDownUpload({ req, res, getSize, download })
+  } catch (err) {
     logger.error(err, 'controller.get.error', req.id)
-    res.status(400).json({ message: 'Failed to download file' })
+    res.status(500).json({ message: 'Failed to download file' })
   }
-
-  startDownUpload({ req, res, getSize, download, onUnhandledError })
 }
 
 module.exports = get

+ 2 - 1
packages/@uppy/companion/src/server/controllers/refresh-token.js

@@ -14,7 +14,8 @@ async function refreshToken (req, res, next) {
   const providerTokens = req.companion.allProvidersTokens[providerName]
 
   // not all providers have refresh tokens
-  if (providerTokens.refreshToken == null) {
+  if (providerTokens.refreshToken == null || providerTokens.refreshToken === '') {
+    logger.warn('Tried to refresh token without having a token')
     res.sendStatus(401)
     return
   }

+ 3 - 3
packages/@uppy/companion/src/server/controllers/url.js

@@ -83,12 +83,12 @@ const get = async (req, res) => {
     return downloadURL(req.body.url, !allowLocalUrls, req.id)
   }
 
-  function onUnhandledError (err) {
+  try {
+    await startDownUpload({ req, res, getSize, download })
+  } catch (err) {
     logger.error(err, 'controller.url.error', req.id)
     res.status(err.status || 500).json({ message: 'failed to fetch URL' })
   }
-
-  startDownUpload({ req, res, getSize, download, onUnhandledError })
 }
 
 module.exports = () => express.Router()

+ 2 - 2
packages/@uppy/companion/src/server/helpers/upload.js

@@ -4,7 +4,7 @@ const { respondWithError } = require('../provider/error')
 
 const { ValidationError } = Uploader
 
-async function startDownUpload ({ req, res, getSize, download, onUnhandledError }) {
+async function startDownUpload ({ req, res, getSize, download }) {
   try {
     const size = await getSize()
     const { clientSocketConnectTimeout } = req.companion.options
@@ -38,7 +38,7 @@ async function startDownUpload ({ req, res, getSize, download, onUnhandledError
 
     if (respondWithError(err, res)) return
 
-    onUnhandledError(err)
+    throw err
   }
 }
 

+ 27 - 8
packages/@uppy/companion/src/server/helpers/utils.js

@@ -148,7 +148,21 @@ module.exports.decrypt = (encrypted, secret) => {
 
 module.exports.defaultGetKey = (req, filename) => `${crypto.randomUUID()}-${filename}`
 
-module.exports.prepareStream = async (stream) => new Promise((resolve, reject) => (
+class StreamHttpJsonError extends Error {
+  statusCode
+
+  responseJson
+
+  constructor({ statusCode, responseJson }) {
+    super(`Request failed with status ${statusCode}`)
+    this.statusCode = statusCode
+    this.responseJson = responseJson
+  }
+}
+
+module.exports.StreamHttpJsonError = StreamHttpJsonError
+
+module.exports.prepareStream = async (stream) => new Promise((resolve, reject) => {
   stream
     .on('response', () => {
       // Don't allow any more data to flow yet.
@@ -157,19 +171,24 @@ module.exports.prepareStream = async (stream) => new Promise((resolve, reject) =
       resolve()
     })
     .on('error', (err) => {
-      // got doesn't parse body as JSON on http error (responseType: 'json' is ignored and it instead becomes a string)
-      if (err?.request?.options?.responseType === 'json' && typeof err?.response?.body === 'string') {
+      // In this case the error object is not a normal GOT HTTPError where json is already parsed,
+      // we create our own StreamHttpJsonError error for this case
+      if (typeof err.response?.body === 'string' && typeof err.response?.statusCode === 'number') {
+        let responseJson
         try {
-          // todo unit test this
-          reject(Object.assign(new Error(), { response: { body: JSON.parse(err.response.body) } }))
+          responseJson = JSON.parse(err.response.body)
         } catch (err2) {
           reject(err)
+          return
         }
-      } else {
-        reject(err)
+
+        reject(new StreamHttpJsonError({ statusCode: err.response.statusCode, responseJson }))
+        return
       }
+
+      reject(err)
     })
-))
+  })
 
 module.exports.getBasicAuthHeader = (key, secret) => {
   const base64 = Buffer.from(`${key}:${secret}`, 'binary').toString('base64')

+ 19 - 0
packages/@uppy/companion/src/server/provider/drive/index.js

@@ -5,6 +5,16 @@ const logger = require('../../logger')
 const { VIRTUAL_SHARED_DIR, adaptData, isShortcut, isGsuiteFile, getGsuiteExportType } = require('./adapter')
 const { withProviderErrorHandling } = require('../providerErrors')
 const { prepareStream } = require('../../helpers/utils')
+const { ProviderAuthError } = require('../error')
+
+
+// For testing refresh token:
+// first run a download with mockAccessTokenExpiredError = true 
+// then when you want to test expiry, set to mockAccessTokenExpiredError to the logged access token
+// This will trigger companion/nodemon to restart, and it will respond with a simulated invalid token response
+const mockAccessTokenExpiredError = undefined
+// const mockAccessTokenExpiredError = true
+// const mockAccessTokenExpiredError = ''
 
 const DRIVE_FILE_FIELDS = 'kind,id,imageMediaMetadata,name,mimeType,ownedByMe,size,modifiedTime,iconLink,thumbnailLink,teamDriveId,videoMediaMetadata,shortcutDetails(targetId,targetMimeType)'
 const DRIVE_FILES_FIELDS = `kind,nextPageToken,incompleteSearch,files(${DRIVE_FILE_FIELDS})`
@@ -117,6 +127,15 @@ class Drive extends Provider {
   }
 
   async download ({ id: idIn, token }) {
+    if (mockAccessTokenExpiredError != null) {
+      logger.warn(`Access token: ${token}`)
+
+      if (mockAccessTokenExpiredError === token) {
+        logger.warn('Mocking expired access token!')
+        throw new ProviderAuthError()
+      }
+    }
+
     return this.#withErrorHandling('provider.drive.download.error', async () => {
       const client = getClient({ token })
 

+ 4 - 0
packages/@uppy/companion/src/server/provider/error.js

@@ -43,6 +43,10 @@ function errorToResponse (err) {
       return { code: 502, message: err.message }
     }
 
+    if (err.statusCode === 429) {
+      return { code: 429, message: err.message }
+    }
+
     if (err.statusCode >= 400) {
       // 424 Failed Dependency
       return { code: 424, message: err.message }

+ 30 - 9
packages/@uppy/companion/src/server/provider/providerErrors.js

@@ -1,6 +1,17 @@
+const { HTTPError } = require('got').default
+
 const logger = require('../logger')
 const { ProviderApiError, ProviderAuthError } = require('./error')
-
+const { StreamHttpJsonError } = require('../helpers/utils')
+
+/**
+ * 
+ * @param {{
+ *   fn: () => any, tag: string, providerName: string, isAuthError: (a: { statusCode: number, body?: object }) => boolean,
+ *   getJsonErrorMessage: (a: object) => string
+ * }} param0 
+ * @returns 
+ */
 async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError = () => false, getJsonErrorMessage }) {
   function getErrorMessage (response) {
     if (typeof response.body === 'object') {
@@ -18,19 +29,29 @@ async function withProviderErrorHandling ({ fn, tag, providerName, isAuthError =
   try {
     return await fn()
   } catch (err) {
-    const { response } = err
+    let statusCode
+    let body
+
+    if (err instanceof HTTPError) {
+      statusCode = err.response?.statusCode
+      body = err.response?.body
+    } else if (err instanceof StreamHttpJsonError) {
+      statusCode = err.statusCode      
+      body = err.responseJson
+    }
 
-    let err2 = err
+    if (statusCode != null) {
+      const err2 = isAuthError({ statusCode, body })
+        ? new ProviderAuthError()
+        : new ProviderApiError(getErrorMessage(body), statusCode)
 
-    if (response) {
-      // @ts-ignore
-      if (isAuthError(response)) err2 = new ProviderAuthError()
-      else err2 = new ProviderApiError(getErrorMessage(response), response.statusCode)
+      logger.error(err2, tag)
+      throw err2
     }
 
-    logger.error(err2, tag)
+    logger.error(err, tag)
 
-    throw err2
+    throw err
   }
 }
 

+ 7 - 7
packages/@uppy/companion/src/server/socket.js

@@ -28,25 +28,25 @@ module.exports = (server) => {
      *
      * @param {{action: string, payload: object}} data
      */
-    function sendProgress (data) {
+    function send (data) {
       ws.send(jsonStringify(data), (err) => {
-        if (err) logger.error(err, 'socket.progress.error', shortenToken(token))
+        if (err) logger.error(err, 'socket.redis.error', shortenToken(token))
       })
     }
 
     // if the redisClient is available, then we attempt to check the storage
-    // if we have any already stored progress data on the upload.
+    // if we have any already stored state on the upload.
     if (redisClient) {
       redisClient.get(`${STORAGE_PREFIX}:${token}`).then((data) => {
         if (data) {
           const dataObj = JSON.parse(data.toString())
-          if (dataObj.action) sendProgress(dataObj)
+          if (dataObj.action) send(dataObj)
         }
       }).catch((err) => logger.error(err, 'socket.redis.error', shortenToken(token)))
     }
-
+ 
     emitter().emit(`connection:${token}`)
-    emitter().on(token, sendProgress)
+    emitter().on(token, send)
 
     ws.on('message', (jsonData) => {
       const data = JSON.parse(jsonData.toString())
@@ -57,7 +57,7 @@ module.exports = (server) => {
     })
 
     ws.on('close', () => {
-      emitter().removeListener(token, sendProgress)
+      emitter().removeListener(token, send)
     })
   })
 }

+ 30 - 0
packages/@uppy/companion/test/__tests__/companion.js

@@ -4,6 +4,7 @@ const request = require('supertest')
 const mockOauthState = require('../mockoauthstate')
 const { version } = require('../../package.json')
 const { nockGoogleDownloadFile } = require('../fixtures/drive')
+const defaults = require('../fixtures/constants')
 
 jest.mock('tus-js-client')
 jest.mock('../../src/server/helpers/oauth-state', () => ({
@@ -45,6 +46,35 @@ afterAll(() => {
 })
 
 describe('validate upload data', () => {
+  test('access token expired or invalid when starting provider download', () => {
+    const meta = {
+      size: null,
+      mimeType: 'video/mp4',
+      id: defaults.ITEM_ID,
+    }
+    nock('https://www.googleapis.com').get(`/drive/v3/files/${defaults.ITEM_ID}`).query(() => true).times(2).reply(200, meta)
+
+    nock('https://www.googleapis.com').get(`/drive/v3/files/${defaults.ITEM_ID}?alt=media&supportsAllDrives=true`).reply(401, {
+      "error": {
+        "code": 401,
+        "message": "Request had invalid authentication credentials. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.",
+        "status": "UNAUTHENTICATED"
+      }
+    })
+  
+    return request(authServer)
+      .post('/drive/get/DUMMY-FILE-ID')
+      .set('uppy-auth-token', token)
+      .set('Content-Type', 'application/json')
+      .send({
+        endpoint: 'http://url.myendpoint.com/files',
+        protocol: 'tus',
+        httpMethod: 'POST',
+      })
+      .expect(401)
+      .then((res) => expect(res.body.message).toBe('HTTP 401: invalid access token detected by Provider'))
+  })
+
   test('invalid upload protocol gets rejected', () => {
     nockGoogleDownloadFile()
 

+ 1 - 0
packages/@uppy/companion/test/__tests__/provider-manager.js

@@ -78,6 +78,7 @@ describe('Test Provider options', () => {
       callback: '/drive/callback',
       custom_params: {
         access_type: 'offline',
+        prompt: 'consent',
       },
     })
     expect(grantConfig.zoom).toEqual({

+ 3 - 1
packages/@uppy/companion/test/fixtures/drive.js

@@ -3,6 +3,8 @@ const defaults = require('./constants')
 
 module.exports.expects = {}
 
+module.exports.nockGoogleDriveAboutCall = () => nock('https://www.googleapis.com').get((uri) => uri.includes('about')).reply(200, { user: { emailAddress: 'john.doe@transloadit.com' } })
+
 module.exports.nockGoogleDownloadFile = ({ times = 1 } = {}) => {
   nock('https://www.googleapis.com').get(`/drive/v3/files/${defaults.ITEM_ID}?fields=kind%2Cid%2CimageMediaMetadata%2Cname%2CmimeType%2CownedByMe%2Csize%2CmodifiedTime%2CiconLink%2CthumbnailLink%2CteamDriveId%2CvideoMediaMetadata%2CshortcutDetails%28targetId%2CtargetMimeType%29&supportsAllDrives=true`).times(times).reply(200, {
     kind: 'drive#file',
@@ -17,5 +19,5 @@ module.exports.nockGoogleDownloadFile = ({ times = 1 } = {}) => {
     size: '758051',
   })
   nock('https://www.googleapis.com').get(`/drive/v3/files/${defaults.ITEM_ID}?alt=media&supportsAllDrives=true`).reply(200, {})
-  nock('https://www.googleapis.com').get((uri) => uri.includes('about')).reply(200, { user: { emailAddress: 'john.doe@transloadit.com' } })
+  module.exports.nockGoogleDriveAboutCall()
 }

+ 6 - 1
packages/@uppy/provider-views/src/View.js

@@ -76,12 +76,17 @@ export default class View {
         body: {
           fileId: file.id,
         },
-        providerOptions: this.provider.opts,
         providerName: this.provider.name,
         provider: this.provider.provider,
       },
     }
 
+    // all properties on this object get saved into the Uppy store.
+    // Some users might serialize their store (for example using JSON.stringify),
+    // or when using Golden Retriever it will serialize state into e.g. localStorage.
+    // However RequestClient is not serializable so we need to prevent it from being serialized.
+    Object.defineProperty(tagFile.remote, 'requestClient', { value: this.provider, enumerable: false })
+
     const fileType = getFileType(tagFile)
 
     // TODO Should we just always use the thumbnail URL if it exists?

+ 0 - 3
packages/@uppy/transloadit/src/index.js

@@ -829,9 +829,6 @@ export default class Transloadit extends BasePlugin {
         // Golden Retriever. So, Golden Retriever is required to do resumability with the Transloadit plugin,
         // and we disable Tus's default resume implementation to prevent bad behaviours.
         storeFingerprintForResuming: false,
-        // Disable Companion'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.
         allowedMetaFields: ['assembly_url', 'filename', 'fieldname'],
         // Pass the limit option to @uppy/tus

+ 2 - 8
packages/@uppy/tus/src/index.js

@@ -1,6 +1,5 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
 import * as tus from 'tus-js-client'
-import { Provider, RequestClient } from '@uppy/companion-client'
 import EventManager from '@uppy/utils/lib/EventManager'
 import NetworkError from '@uppy/utils/lib/NetworkError'
 import isNetworkError from '@uppy/utils/lib/isNetworkError'
@@ -67,7 +66,6 @@ export default class Tus extends BasePlugin {
 
     // set default options
     const defaultOptions = {
-      useFastRemoteRetry: true,
       limit: 20,
       retryDelays: tusDefaultOptions.retryDelays,
       withCredentials: false,
@@ -483,11 +481,7 @@ export default class Tus extends BasePlugin {
       const total = files.length
 
       if (file.isRemote) {
-        // INFO: the url plugin needs to use RequestClient,
-        // while others use Provider
-        const Client = file.remote.providerOptions.provider ? Provider : RequestClient
         const getQueue = () => this.requests
-        const client = new Client(this.uppy, file.remote.providerOptions, getQueue)
         const controller = new AbortController()
 
         const removedHandler = (removedFile) => {
@@ -495,10 +489,10 @@ export default class Tus extends BasePlugin {
         }
         this.uppy.on('file-removed', removedHandler)
 
-        const uploadPromise = client.uploadRemoteFile(
+        const uploadPromise = file.remote.requestClient.uploadRemoteFile(
           file,
           this.#getCompanionClientArgs(file),
-          { signal: controller.signal },
+          { signal: controller.signal, getQueue },
         )
 
         this.requests.wrapSyncFunction(() => {

+ 0 - 1
packages/@uppy/tus/types/index.d.ts

@@ -27,7 +27,6 @@ type Next = (
 export interface TusOptions extends PluginOptions, TusUploadOptions {
   allowedMetaFields?: string[] | null
   limit?: number
-  useFastRemoteRetry?: boolean
   withCredentials?: boolean
   onShouldRetry?: (
     err: Error | undefined,

+ 3 - 1
packages/@uppy/url/src/Url.jsx

@@ -132,9 +132,11 @@ export default class Url extends UIPlugin {
             fileId: url,
             url,
           },
-          providerOptions: this.client.opts,
         },
       }
+
+      Object.defineProperty(tagFile.remote, 'requestClient', { value: this.client, enumerable: false })
+
       this.uppy.log('[Url] Adding remote file')
       try {
         return this.uppy.addFile(tagFile)

+ 1 - 1
packages/@uppy/utils/src/RateLimitedQueue.js

@@ -54,7 +54,7 @@ export class RateLimitedQueue {
         if (done) return
         done = true
         this.#activeRequests -= 1
-        cancelActive(cause)
+        cancelActive?.(cause)
         this.#queueNext()
       },
 

+ 1 - 1
packages/@uppy/utils/src/fetchWithNetworkError.js

@@ -6,7 +6,7 @@ import NetworkError from './NetworkError.js'
 export default function fetchWithNetworkError (...options) {
   return fetch(...options)
     .catch((err) => {
-      if (err.name === 'AbortError') {
+      if (err.name === 'AbortError') { // todo maybe instead see npm package is-network-error
         throw err
       } else {
         throw new NetworkError(err)

+ 2 - 7
packages/@uppy/xhr-upload/src/index.js

@@ -1,6 +1,5 @@
 import BasePlugin from '@uppy/core/lib/BasePlugin.js'
 import { nanoid } from 'nanoid/non-secure'
-import { Provider, RequestClient } from '@uppy/companion-client'
 import EventManager from '@uppy/utils/lib/EventManager'
 import ProgressTimeout from '@uppy/utils/lib/ProgressTimeout'
 import { RateLimitedQueue, internalRateLimitedQueue } from '@uppy/utils/lib/RateLimitedQueue'
@@ -466,11 +465,7 @@ export default class XHRUpload extends BasePlugin {
       const total = files.length
 
       if (file.isRemote) {
-        // INFO: the url plugin needs to use RequestClient,
-        // while others use Provider
-        const Client = file.remote.providerOptions.provider ? Provider : RequestClient
         const getQueue = () => this.requests
-        const client = new Client(this.uppy, file.remote.providerOptions, getQueue)
         const controller = new AbortController()
 
         const removedHandler = (removedFile) => {
@@ -478,10 +473,10 @@ export default class XHRUpload extends BasePlugin {
         }
         this.uppy.on('file-removed', removedHandler)
 
-        const uploadPromise = client.uploadRemoteFile(
+        const uploadPromise = file.remote.requestClient.uploadRemoteFile(
           file,
           this.#getCompanionClientArgs(file),
-          { signal: controller.signal },
+          { signal: controller.signal, getQueue },
         )
 
         this.requests.wrapSyncFunction(() => {

+ 26 - 0
yarn.lock

@@ -8370,6 +8370,13 @@ __metadata:
   languageName: node
   linkType: hard
 
+"@types/retry@npm:0.12.2":
+  version: 0.12.2
+  resolution: "@types/retry@npm:0.12.2"
+  checksum: e5675035717b39ce4f42f339657cae9637cf0c0051cf54314a6a2c44d38d91f6544be9ddc0280587789b6afd056be5d99dbe3e9f4df68c286c36321579b1bf4a
+  languageName: node
+  linkType: hard
+
 "@types/sass@npm:^1.16.0":
   version: 1.43.1
   resolution: "@types/sass@npm:1.43.1"
@@ -9282,6 +9289,7 @@ __metadata:
   dependencies:
     "@uppy/utils": "workspace:^"
     namespace-emitter: ^2.0.1
+    p-retry: ^6.1.0
     vitest: ^0.34.5
   languageName: unknown
   linkType: soft
@@ -19021,6 +19029,13 @@ __metadata:
   languageName: node
   linkType: hard
 
+"is-network-error@npm:^1.0.0":
+  version: 1.0.0
+  resolution: "is-network-error@npm:1.0.0"
+  checksum: 2ca2b4b2d420015e0237abe28ebf316fcd26a82304b07432abf155759a3bee6895609ac91e692a72ad61b7fc902c3283b2dece61e1ddb05a6257777a8573e468
+  languageName: node
+  linkType: hard
+
 "is-number-object@npm:^1.0.4":
   version: 1.0.7
   resolution: "is-number-object@npm:1.0.7"
@@ -24681,6 +24696,17 @@ __metadata:
   languageName: node
   linkType: hard
 
+"p-retry@npm:^6.1.0":
+  version: 6.1.0
+  resolution: "p-retry@npm:6.1.0"
+  dependencies:
+    "@types/retry": 0.12.2
+    is-network-error: ^1.0.0
+    retry: ^0.13.1
+  checksum: 1083b2b72672205680f8a736583e31dce5d4ae472996cd06f4a33cd7ea11798d7712c202d253eb8afbdc80abf52f049651989c59f2e2ccca529e6b64d722b1f7
+  languageName: node
+  linkType: hard
+
 "p-timeout@npm:^5.0.2":
   version: 5.1.0
   resolution: "p-timeout@npm:5.1.0"