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

@uppy/aws-s3-multipart: add shouldUseMultipart option #4205

Merged
merged 16 commits into from
May 2, 2023
Merged
39 changes: 22 additions & 17 deletions packages/@uppy/aws-s3-multipart/src/MultipartUploader.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class MultipartUploader {

#onSuccess

#shouldUseMultipart

#onReject = (err) => (err?.cause === pausingUploadReason ? null : this.#onError(err))

constructor (data, options) {
Expand All @@ -57,25 +59,23 @@ class MultipartUploader {
this.#file = options.file
this.#onSuccess = this.options.onSuccess
this.#onError = this.options.onError
this.#shouldUseMultipart = this.options.shouldUseMultipart

this.#initChunks()
}

#initChunks () {
const desiredChunkSize = this.options.getChunkSize(this.#data)
// at least 5MB per request, at most 10k requests
const fileSize = this.#data.size
const minChunkSize = Math.max(5 * MB, Math.ceil(fileSize / 10000))
const chunkSize = Math.max(desiredChunkSize, minChunkSize)
const shouldUseMultipart = typeof this.#shouldUseMultipart === 'function'
? this.#shouldUseMultipart(this.#file, fileSize)
: Boolean(this.#shouldUseMultipart)

if (shouldUseMultipart) {
const desiredChunkSize = this.options.getChunkSize(this.#data)
// at least 5MB per request, at most 10k requests
const minChunkSize = Math.max(5 * MB, Math.ceil(fileSize / 10000))
const chunkSize = Math.max(desiredChunkSize, minChunkSize)

// Upload zero-sized files in one zero-sized chunk
if (this.#data.size === 0) {
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
this.#chunks = [{
getData: () => this.#data,
onProgress: this.#onPartProgress(0),
onComplete: this.#onPartComplete(0),
}]
} else {
const arraySize = Math.ceil(fileSize / chunkSize)
this.#chunks = Array(arraySize)

Expand All @@ -92,8 +92,16 @@ class MultipartUploader {
getData,
onProgress: this.#onPartProgress(j),
onComplete: this.#onPartComplete(j),
shouldUseMultipart,
}
}
} else {
this.#chunks = [{
getData: () => this.#data,
onProgress: this.#onPartProgress(0),
onComplete: this.#onPartComplete(0),
shouldUseMultipart,
}]
}

this.#chunkState = this.#chunks.map(() => ({ uploaded: 0 }))
Expand All @@ -111,11 +119,8 @@ class MultipartUploader {
.then(this.#onSuccess, this.#onReject)
}

#onPartProgress = (index) => (ev) => {
if (!ev.lengthComputable) return

const sent = ev.loaded
this.#chunkState[index].uploaded = ensureInt(sent)
#onPartProgress = (index) => (bytes) => {
this.#chunkState[index].uploaded = ensureInt(bytes)

const totalUploaded = this.#chunkState.reduce((n, c) => n + c.uploaded, 0)
this.options.onProgress(totalUploaded, this.#data.size)
Expand Down
61 changes: 56 additions & 5 deletions packages/@uppy/aws-s3-multipart/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class HTTPCommunicationQueue {

#fetchSignature

#getUploadParameters

#listParts

#previousRetryDelay
Expand Down Expand Up @@ -75,6 +77,9 @@ class HTTPCommunicationQueue {
if ('uploadPartBytes' in options) {
this.#uploadPartBytes = requests.wrapPromiseFunction(options.uploadPartBytes, { priority:Infinity })
}
if ('getUploadParameters' in options) {
this.#getUploadParameters = requests.wrapPromiseFunction(options.getUploadParameters)
}
}

async #shouldRetry (err) {
Expand Down Expand Up @@ -190,8 +195,41 @@ class HTTPCommunicationQueue {
await this.#abortMultipartUpload(file, awaitedResult)
}

async #nonMultipartUpload (file, chunk, signal) {
const filename = file.meta.name
const { type } = file.meta
const metadata = undefined // TODO: add `allowedMetaFields` option to S3 multipart?
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

const query = new URLSearchParams({ filename, type, ...metadata })
const {
method = 'post',
url,
fields,
headers,
} = await this.#getUploadParameters(`s3/params?${query}`, { signal }).abortOn(signal)

const formData = new FormData()
Object.entries(fields).forEach(([key, value]) => formData.set(key, value))
const data = chunk.getData()
formData.set('file', data)

const { onProgress, onComplete } = chunk

return this.#uploadPartBytes({
signature: { url, headers, method },
body: formData,
size: data.size,
onProgress,
onComplete,
signal,
}).abortOn(signal)
}

async uploadFile (file, chunks, signal) {
throwIfAborted(signal)
if (chunks.length === 1 && !chunks[0].shouldUseMultipart) {
return this.#nonMultipartUpload(file, chunks[0], signal)
}
const { uploadId, key } = await this.getUploadId(file, signal)
throwIfAborted(signal)
try {
Expand All @@ -206,6 +244,9 @@ class HTTPCommunicationQueue {

async resumeUploadFile (file, chunks, signal) {
throwIfAborted(signal)
if (chunks.length === 1) {

Choose a reason for hiding this comment

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

return this.#nonMultipartUpload(file, chunks[0], signal)
}
const { uploadId, key } = await this.getUploadId(file, signal)
throwIfAborted(signal)
const alreadyUploadedParts = await this.#listParts(file, { uploadId, key, signal }).abortOn(signal)
Expand Down Expand Up @@ -240,7 +281,9 @@ class HTTPCommunicationQueue {
try {
return {
PartNumber: partNumber,
...await this.#uploadPartBytes({ signature, body: chunkData, onProgress, onComplete, signal }).abortOn(signal),
...await this.#uploadPartBytes({
signature, body: chunkData, size: chunkData.size, onProgress, onComplete, signal,
}).abortOn(signal),
}
} catch (err) {
if (!await this.#shouldRetry(err)) throw err
Expand Down Expand Up @@ -269,13 +312,17 @@ export default class AwsS3Multipart extends BasePlugin {
// TODO: this is currently opt-in for backward compat, switch to opt-out in the next major
allowedMetaFields: null,
limit: 6,
shouldUseMultipart: true, // In the next major, the default should be switched as the following:
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line no-bitwise
// shouldUseMultipart: (file, fileSize) => fileSize >> 10 >> 10 > 100,
retryDelays: [0, 1000, 3000, 5000],
createMultipartUpload: this.createMultipartUpload.bind(this),
listParts: this.listParts.bind(this),
abortMultipartUpload: this.abortMultipartUpload.bind(this),
completeMultipartUpload: this.completeMultipartUpload.bind(this),
signPart: this.signPart.bind(this),
uploadPartBytes: AwsS3Multipart.uploadPartBytes,
getUploadParameters: (...args) => this.#client.get(...args),
companionHeaders: {},
}

Expand Down Expand Up @@ -397,7 +444,7 @@ export default class AwsS3Multipart extends BasePlugin {
.then(assertServerError)
}

static async uploadPartBytes ({ signature: { url, expires, headers }, body, onProgress, onComplete, signal }) {
static async uploadPartBytes ({ signature: { url, expires, headers, method = 'PUT' }, body, size, onProgress, onComplete, signal }) {
throwIfAborted(signal)

if (url == null) {
Expand All @@ -406,7 +453,7 @@ export default class AwsS3Multipart extends BasePlugin {

return new Promise((resolve, reject) => {
const xhr = new XMLHttpRequest()
xhr.open('PUT', url, true)
xhr.open(method, url, true)
if (headers) {
Object.keys(headers).forEach((key) => {
xhr.setRequestHeader(key, headers[key])
Expand All @@ -425,7 +472,10 @@ export default class AwsS3Multipart extends BasePlugin {
}
signal.addEventListener('abort', onabort)

xhr.upload.addEventListener('progress', onProgress)
xhr.upload.addEventListener('progress', (ev) => {
if (!ev.lengthComputable) return
onProgress(ev.loaded)
})

xhr.addEventListener('abort', () => {
cleanup()
Expand Down Expand Up @@ -455,7 +505,7 @@ export default class AwsS3Multipart extends BasePlugin {
return
}

onProgress?.(body.size)
onProgress?.(size)

// NOTE This must be allowed by CORS.
const etag = ev.target.getResponseHeader('ETag')
Expand Down Expand Up @@ -550,6 +600,7 @@ export default class AwsS3Multipart extends BasePlugin {
onPartComplete,

file,
shouldUseMultipart: this.opts.shouldUseMultipart,

...file.s3Multipart,
})
Expand Down
1 change: 1 addition & 0 deletions packages/@uppy/aws-s3-multipart/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface AwsS3MultipartOptions extends PluginOptions {
opts: { uploadId: string; key: string; parts: AwsS3Part[]; signal: AbortSignal }
) => MaybePromise<{ location?: string }>
limit?: number
shouldUseMultipart?: boolean | ((file: UppyFile, fileSize: number) => boolean)
Copy link
Member

Choose a reason for hiding this comment

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

Why is fileSize the second argument? Doesn't it already live in file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's provided as a convenience.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's cleaner to just have file. It's not some deep object, it's simply file.size. We can add another argument in the future when people start using it more and need more things or conveniences.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that if UppyFile always has a .size, then it's unnecessary/confusing/redundant to have a separate argument for it as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed.

retryDelays?: number[] | null
}

Expand Down