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

Retry prepareUploadParts on fail for @uppy/aws-s3-multipart #3224

Merged
merged 8 commits into from Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
Expand Up @@ -244,18 +244,20 @@ class MultipartUploader {
async #prepareUploadParts (candidates) {
this.lockedCandidatesForBatch.push(...candidates)

const result = await this.options.prepareUploadParts({
key: this.key,
uploadId: this.uploadId,
partNumbers: candidates.map((index) => index + 1),
const result = await this.#retryable({
attempt: () => this.options.prepareUploadParts({
key: this.key,
uploadId: this.uploadId,
partNumbers: candidates.map((index) => index + 1),
}),
})

const valid = typeof result?.presignedUrls === 'object'
if (!valid) {
if (typeof result?.presignedUrls !== 'object') {
throw new TypeError(
'AwsS3/Multipart: Got incorrect result from `prepareUploadParts()`, expected an object `{ presignedUrls }`.'
)
}

return result
}

Expand Down
138 changes: 97 additions & 41 deletions packages/@uppy/aws-s3-multipart/src/index.test.js
Expand Up @@ -50,9 +50,9 @@ describe('AwsS3Multipart', () => {
key: 'test/upload/multitest.dat',
}
}),
completeMultipartUpload: jest.fn(() => Promise.resolve({ location: 'test' })),
completeMultipartUpload: jest.fn(async () => ({ location: 'test' })),
abortMultipartUpload: jest.fn(),
prepareUploadParts: jest.fn(() => {
prepareUploadParts: jest.fn(async () => {
const presignedUrls = {}
const possiblePartNumbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
possiblePartNumbers.forEach((partNumber) => {
Expand All @@ -66,14 +66,18 @@ describe('AwsS3Multipart', () => {
awsS3Multipart = core.getPlugin('AwsS3Multipart')
})

it('Calls the prepareUploadParts function totalChunks / limit times', (done) => {
it('Calls the prepareUploadParts function totalChunks / limit times', async () => {
const scope = nock(
'https://bucket.s3.us-east-2.amazonaws.com'
).defaultReplyHeaders({
'access-control-allow-method': 'PUT',
'access-control-allow-origin': '*',
'access-control-expose-headers': 'ETag',
})
// 6MB file will give us 2 chunks, so there will be 2 PUT and 2 OPTIONS
// calls to the presigned URL from 1 prepareUploadParts calls
const fileSize = 5 * MB + 1 * MB

scope
.options((uri) => uri.includes('test/upload/multitest.dat'))
.reply(200, '')
Expand All @@ -87,9 +91,6 @@ describe('AwsS3Multipart', () => {
.put((uri) => uri.includes('test/upload/multitest.dat'))
.reply(200, '', { ETag: 'test2' })

// 6MB file will give us 2 chunks, so there will be 2 PUT and 2 OPTIONS
// calls to the presigned URL from 1 prepareUploadParts calls
const fileSize = 5 * MB + 1 * MB
core.addFile({
source: 'jest',
name: 'multitest.dat',
Expand All @@ -98,23 +99,31 @@ describe('AwsS3Multipart', () => {
type: 'application/octet-stream',
}),
})
core.upload().then(() => {
expect(
awsS3Multipart.opts.prepareUploadParts.mock.calls.length
).toEqual(1)
scope.done()
done()
})

await core.upload()

expect(
awsS3Multipart.opts.prepareUploadParts.mock.calls.length
).toEqual(1)

scope.done()
})

it('Calls prepareUploadParts with a Math.ceil(limit / 2) minimum, instead of one at a time for the remaining chunks after the first limit batch', (done) => {
it('Calls prepareUploadParts with a Math.ceil(limit / 2) minimum, instead of one at a time for the remaining chunks after the first limit batch', async () => {
const scope = nock(
'https://bucket.s3.us-east-2.amazonaws.com'
).defaultReplyHeaders({
'access-control-allow-method': 'PUT',
'access-control-allow-origin': '*',
'access-control-expose-headers': 'ETag',
})
// 50MB file will give us 10 chunks, so there will be 10 PUT and 10 OPTIONS
// calls to the presigned URL from 3 prepareUploadParts calls
//
// The first prepareUploadParts call will be for 5 parts, the second
// will be for 3 parts, the third will be for 2 parts.
const fileSize = 50 * MB

scope
.options((uri) => uri.includes('test/upload/multitest.dat'))
.reply(200, '')
Expand All @@ -123,12 +132,6 @@ describe('AwsS3Multipart', () => {
.reply(200, '', { ETag: 'test' })
scope.persist()

// 50MB file will give us 10 chunks, so there will be 10 PUT and 10 OPTIONS
// calls to the presigned URL from 3 prepareUploadParts calls
//
// The first prepareUploadParts call will be for 5 parts, the second
// will be for 3 parts, the third will be for 2 parts.
const fileSize = 50 * MB
core.addFile({
source: 'jest',
name: 'multitest.dat',
Expand All @@ -137,28 +140,81 @@ describe('AwsS3Multipart', () => {
type: 'application/octet-stream',
}),
})
core.upload().then(() => {
expect(
awsS3Multipart.opts.prepareUploadParts.mock.calls.length
).toEqual(3)
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[0][1].partNumbers).toEqual([1, 2, 3, 4, 5])
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[1][1].partNumbers).toEqual([6, 7, 8])
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[2][1].partNumbers).toEqual([9, 10])
const completeCall = awsS3Multipart.opts.completeMultipartUpload.mock.calls[0][1]
expect(completeCall.parts).toEqual([
{ ETag: 'test', PartNumber: 1 },
{ ETag: 'test', PartNumber: 2 },
{ ETag: 'test', PartNumber: 3 },
{ ETag: 'test', PartNumber: 4 },
{ ETag: 'test', PartNumber: 5 },
{ ETag: 'test', PartNumber: 6 },
{ ETag: 'test', PartNumber: 7 },
{ ETag: 'test', PartNumber: 8 },
{ ETag: 'test', PartNumber: 9 },
{ ETag: 'test', PartNumber: 10 },
])
done()

await core.upload()

expect(awsS3Multipart.opts.prepareUploadParts.mock.calls.length).toEqual(3)
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[0][1].partNumbers).toEqual([1, 2, 3, 4, 5])
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[1][1].partNumbers).toEqual([6, 7, 8])
expect(awsS3Multipart.opts.prepareUploadParts.mock.calls[2][1].partNumbers).toEqual([9, 10])

const completeCall = awsS3Multipart.opts.completeMultipartUpload.mock.calls[0][1]

expect(completeCall.parts).toEqual([
{ ETag: 'test', PartNumber: 1 },
{ ETag: 'test', PartNumber: 2 },
{ ETag: 'test', PartNumber: 3 },
{ ETag: 'test', PartNumber: 4 },
{ ETag: 'test', PartNumber: 5 },
{ ETag: 'test', PartNumber: 6 },
{ ETag: 'test', PartNumber: 7 },
{ ETag: 'test', PartNumber: 8 },
{ ETag: 'test', PartNumber: 9 },
{ ETag: 'test', PartNumber: 10 },
])
})
})

describe('MultipartUploader', () => {
let core
let awsS3Multipart

beforeEach(() => {
core = new Core()
core.use(AwsS3Multipart, {
createMultipartUpload: jest.fn(() => {
return {
uploadId: '6aeb1980f3fc7ce0b5454d25b71992',
key: 'test/upload/multitest.dat',
}
}),
completeMultipartUpload: jest.fn(async () => ({ location: 'test' })),
abortMultipartUpload: jest.fn(),
prepareUploadParts: jest
.fn(async () => {
const presignedUrls = {}
const possiblePartNumbers = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

possiblePartNumbers.forEach((partNumber) => {
presignedUrls[
partNumber
] = `https://bucket.s3.us-east-2.amazonaws.com/test/upload/multitest.dat?partNumber=${partNumber}&uploadId=6aeb1980f3fc7ce0b5454d25b71992&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIATEST%2F20210729%2Fus-east-2%2Fs3%2Faws4_request&X-Amz-Date=20210729T014044Z&X-Amz-Expires=600&X-Amz-SignedHeaders=host&X-Amz-Signature=test`
})

return { presignedUrls }
})
// This runs first and only once
// eslint-disable-next-line prefer-promise-reject-errors
.mockImplementationOnce(() => Promise.reject({ source: { status: 500 } })),
})
awsS3Multipart = core.getPlugin('AwsS3Multipart')
})

it('retries prepareUploadParts when it fails once', async () => {
const fileSize = 5 * MB + 1 * MB

core.addFile({
source: 'jest',
name: 'multitest.dat',
type: 'application/octet-stream',
data: new File([Buffer.alloc(fileSize)], {
type: 'application/octet-stream',
}),
})

await core.upload()

expect(awsS3Multipart.opts.prepareUploadParts.mock.calls.length).toEqual(2)
})
})
})
35 changes: 24 additions & 11 deletions website/src/docs/aws-s3-multipart.md
Expand Up @@ -47,7 +47,9 @@ For example, with a 50MB file and a `limit` of 5 we end up with 10 chunks. 5 of

### `retryDelays: [0, 1000, 3000, 5000]`

When uploading a chunk to S3 using a presigned URL fails, automatically try again after the millisecond intervals specified in this array. By default, we first retry instantly; if that fails, we retry after 1 second; if that fails, we retry after 3 seconds, etc.
`retryDelays` are the intervals in milliseconds used to retry a failed chunk as well as [`prepareUploadParts`](#prepareUploadParts-file-partData).

By default, we first retry instantly; if that fails, we retry after 1 second; if that fails, we retry after 3 seconds, etc.

Set to `null` to disable automatic retries, and fail instantly if any chunk fails to upload.

Expand Down Expand Up @@ -109,20 +111,31 @@ A function that generates a batch of signed URLs for the specified part numbers.
- `key` - The object key in the S3 bucket.
- `partNumbers` - An array of indecies of this part in the file (`PartNumber` in S3 terminology). Note that part numbers are _not_ zero-based.

Return a Promise for an object with keys:
`prepareUploadParts` should return a `Promise` with an `Object` with keys:

- `presignedUrls` - A JavaScript object with the part numbers as keys and the presigned URL for each part as the value. An example of what the return value should look like:
Murderlon marked this conversation as resolved.
Show resolved Hide resolved

```js
// for partNumbers [1, 2, 3]
return {
1: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...',
2: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...',
3: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&...',
}
```
- `headers` - **(Optional)** Custom headers that should be sent to the S3 presigned URL.

Murderlon marked this conversation as resolved.
Show resolved Hide resolved
```json
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
{
"presignedUrls": {
"1": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...",
"2": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...",
"3": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&..."
},
"headers": { "some-header": "value" }
}
Comment on lines +121 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
"presignedUrls": {
"1": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...",
"2": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...",
"3": "https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&..."
},
"headers": { "some-header": "value" }
}
{
presignedUrls: {
1: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=1&...',
2: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=2&...',
3: 'https://bucket.region.amazonaws.com/path/to/file.jpg?partNumber=3&...',
},
headers: { 'some-header': 'value' },
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the linter is going to like that. FWIW I think JSON is fine here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't know that we also lint js blocks inside markdown.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also inclined to keep it the way it is.

```

If an error occured, reject the `Promise` with an `Object` with the following keys:

<!-- eslint-disable -->
```json
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
{ "source": { "status": 500 } }
Murderlon marked this conversation as resolved.
Show resolved Hide resolved
```

`status` is the HTTP code and is required for determining whether to retry the request. `prepareUploadParts` will be retried if the code is `0`, `409`, `423`, or between `500` and `600`.

### `abortMultipartUpload(file, { uploadId, key })`

A function that calls the S3 Multipart API to abort a Multipart upload, and delete all parts that have been uploaded so far. Receives the `file` object from Uppy's state, and an object with keys:
Expand Down