Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tus: 500 response cause duplicate upload requests #3594

Closed
Acconut opened this issue Mar 23, 2022 · 0 comments · Fixed by #3595
Closed

tus: 500 response cause duplicate upload requests #3594

Acconut opened this issue Mar 23, 2022 · 0 comments · Fixed by #3595
Assignees
@aduh95
Labels
Bug

Comments

@Acconut
Copy link
Member

Acconut commented Mar 23, 2022 •
edited
Loading

While debugging upload issues for Transloadit, I found following bug: If the server responds with a 500 error, Uppy causes two HEAD requests to be sent followed by two PATCH requests, instead of only one. This is problematic because tus server do not allow concurrent requests for a single upload.

I was able to reproduce it locally by following steps:

  1. Download the build of the latest tusd version from https://github.com/tus/tusd/releases/tag/v2.0.0-rc8
  2. Start tusd, but set a low timeout (this timeout does not make sense in production but it a nice way to consistently reproduce the error): tusd -timeout=500
  3. Open the HTML code from below in your browser.
  4. Open the network tab in your brower's dev tools and enable throttling. In Firefox, I used the GPRS setting, which limits the upload speed to 20 Kbps (https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/devtools/client/shared/components/throttling/profiles.js#22). For Chrome, I added a custom throttling to limit upload speed to 20Kbps. This low upload speed in conjunction with the low read timeout of tusd, causes tusd to respond with a 500 read timeout error consistently.
  5. Begin the upload and observe following behavior in the network tab:
  • At first, a POST request is sent to create the upload. That works fine.
  • Then, the first PATCH request is sent, which gets aborted by the 500 error as mentioned above.
  • Then, two HEAD requests are sent. Only one HEAD request should be sent, but they are duplicate.
  • The two HEAD requests cause two PATCH requests to be sent. Only one should occur.
  • This sequence repeats because the new PATCH requests also get a 500 response.

I also tested the same setup using only tus-js-client and not Uppy. I did not observe this behavior, so I believe it to be caused by Uppy (although tus-js-client could implement a prevention for this, as I will describe later).

The expected behavior is:

  • Uppy should only send one pair of HEAD and PATCH request.

The full Uppy code is this:

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width">
  <title>Uppy & tus example</title>
  <link href="https://releases.transloadit.com/uppy/v2.8.0/uppy.min.css" rel="stylesheet">
</head>
<body>
  <div id="uppy-dashboard-container"></div>
  <div id="uppy-transloadit-result"></div>

  <script src="https://releases.transloadit.com/uppy/v2.8.0/uppy.min.js"></script>
  <script>
    const uppy = new Uppy.Core({
      debug: true,
      autoProceed: false
    })

    uppy
      .use(Uppy.Tus, {
        endpoint: 'http://localhost:1080/files/',
      })
      .use(Uppy.Dashboard, {
        inline: true,
        maxHeight: 400,
        target: '#uppy-dashboard-container'
      })
  </script>

</body>
</html>

I debugged a bit and found following hints:

  • If an error occurs, tus-js-client calls Uppy's onShouldRetry callback. For a 500 error following condition is executed:

    uppy/packages/@uppy/tus/src/index.js

    Lines 290 to 293 in e80a7f7

    setTimeout(() => {
    queuedRequest.abort()
    queuedRequest = this.requests.run(qRequest)
    }, options.retryDelays[retryAttempt])
  • If I understand correctly, this causes a new call to upload.start() to be called in conjunction with:

    uppy/packages/@uppy/tus/src/index.js

    Lines 328 to 339 in e80a7f7

    qRequest = () => {
    if (!file.isPaused) {
    upload.start()
    }
    // Don't do anything here, the caller will take care of cancelling the upload itself
    // using resetUploaderReferences(). This is because resetUploaderReferences() has to be
    // called when this request is still in the queue, and has not been started yet, too. At
    // that point this cancellation function is not going to be called.
    // Also, we need to remove the request from the queue _without_ destroying everything
    // related to this upload to handle pauses.
    return () => {}
    }
  • However, onShouldRetry also returns true, instructing tus-js-client to retry the uploads:

    uppy/packages/@uppy/tus/src/index.js

    Line 299 in e80a7f7

    return true

In effect, tus-js-client is told to retry the upload (causing one pair of HEAD/PATCH requests), while also upload.start is called, causing another pair of requests. tus-js-client currently does not have protection against calling upload.start while the upload is already running. We should implement this, but Uppy's current behavior also seems suspicious because requests are sent, without going through Uppy's request queue.

I believe customers are currently suffering from this issue, so a quick help is appreciated :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aduh95 aduh95

Labels
Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

@uppy/tus: fix double requests sent when rate limiting transloadit/uppy
2 participants
@Acconut @aduh95