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.

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

Retrying aws multipart upload doesn't work #4366

Closed
2 tasks done
pjamrozowicz opened this issue Mar 19, 2023 · 5 comments · Fixed by #4424
Closed
2 tasks done

Retrying aws multipart upload doesn't work #4366

pjamrozowicz opened this issue Mar 19, 2023 · 5 comments · Fixed by #4424
Assignees
Labels
Bug prio-1 High-priority issues

Comments

@pjamrozowicz
Copy link

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

Calling retryUpload(fileId) doesn't actually try to call the server again, it just reuses last response.

I'm not entirely sure if this #4313 is related or not.

In my case:

  • Uppy setup is:
const uppy = new Uppy({
    id: 'test',
    autoProceed: true,
    allowMultipleUploadBatches: true,
    restrictions: {
      maxFileSize: null,
      maxTotalFileSize: null,
    },
  });

uppy.use(AwsS3Multipart, {
  limit: 4,
  companionUrl: '/api/storage/',
});

Then:

// 1. Add some file to upload
uppy.addFile(...)

// 2. My server returns 500 error (as expected)

// 3. I try to retry the upload
uppy.retryUpload(fileId)

// 4. It doesn't hit the server again, it just reuses the previous response.

I found the problematic code is located in @uppy/aws-s3-multipart index.js file:

  async getUploadId(file, signal) {
    const cachedResult = this.#cache.get(file.data);

    if (cachedResult != null) {
      return cachedResult;
    }

    const promise = this.#createMultipartUpload(file, signal);

    (...)
}

The cachedResult contains a rejected promise so the logic above doesn't get past that if, it early returns.

Expected behavior

The cache value should either be skipped or removed when retryUpload is called.

Actual behavior

Retry doesn't actually retry anything, it just reuses last server response.

@mifi
Copy link
Contributor

mifi commented Mar 23, 2023

just to be sure, you're uploading a local file, right? not from a companion provider? linking a similar issue here #4356 - but I think it might be unrelated because that issue has to do with companion providers

@pjamrozowicz
Copy link
Author

just to be sure, you're uploading a local file, right? not from a companion provider? linking a similar issue here #4356 - but I think it might be unrelated because that issue has to do with companion

Yes, exactly, a local file. It's definitely an issue with the multipart companion.

@mifi
Copy link
Contributor

mifi commented Mar 24, 2023

I tried to reproduce this issue using the UI, but retrying works for me. is the problem only when using the API?

Also you say that:

// 1. Add some file to upload
uppy.addFile(...)
// 2. My server returns 500 error (as expected)

I believe that addFile doesn't actually start an upload, but it just adds the file to the UI, so I'm not sure why your server would return a 500 in this case

@snoobism
Copy link

snoobism commented Apr 11, 2023

I am also encountering this issue when retrying failed multipart uploads. Is there a way of clearing the cache or some workaround?

@pjamrozowicz
Copy link
Author

I tried to reproduce this issue using the UI, but retrying works for me. is the problem only when using the API?

Also you say that:

// 1. Add some file to upload
uppy.addFile(...)
// 2. My server returns 500 error (as expected)

I believe that addFile doesn't actually start an upload, but it just adds the file to the UI, so I'm not sure why your server would return a 500 in this case

It does start the upload if you set autoProceed: true.

I'm not sure what you mean by UI vs API. I'm using the library with my own UI but I don't think that should be a problem since a list of steps to reproduce is really straightforward without ~any UI.


@snoobism as a workaround I copied the aws-s3-multipart code since it's quite small, then:

In the index.js file, inside HTTPCommunicationQueue I added function:

async resetUpload(file) {
  this.#cache.delete(file.data);
}

Next, inside the AwsS3Multipart class, inside uploadFile function, I updated the onError callback to look like this:

const onError = (err) => {
  this.uppy.log(err);
  this.uppy.emit('upload-error', file, err);

  this.resetUploaderReferences(file.id);
  this.#companionCommunicationQueue.resetUpload(file); // <- this line is new
  reject(err);
};

As far as I remember - that's it. Retrying works for me with these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug prio-1 High-priority issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants