@uppy/aws-s3-multipart: increase priority of abort and complete

[schonert]

Closes #4541
Closes #4326

Wraps the final upload requests, abortMultipartUpload and completeMultipartUpload, in a separate queue. Preventing the race condition where compiled uploads would stale if new uploads filled up the queue.

Considered simply rewriting the areas that used the methods to not follow the queue pattern (onAbort etc). However, this seemed like a less intrusive solution allowing for a more permanent solution to evolve down the road.

Also, you should consider adding some sort of format config (eg. prettier) 😄

[aduh95]

I don't think that's the correct fix, we would be doubling the number of requests made in parallel, making limit very hard to document and confusing for users. I think we should instead change the priority of those requests, on the same queue.

[schonert]

I think we should instead change the priority of those requests, on the same queue.

@aduh95 I don't think this will solve the race-conditions?
A completion request will still have to wait for a spot in the queue - which can be running on all cylinders. So even if it's the highest priority it will have to wait for an opening to appear.

[Murderlon]

So even if it's the highest priority it will have to wait for an opening to appear.

but that doesn't matter, right? Nothing goes wrong if we don't call finish immediately after the last patch request. We simply want to make sure it isn't postponed for too long. Furthermore, the idea of a priority queue is that if you manage it well it should behave in the order you want it to happen. It simply has to be the highest priority at the right time.

[aduh95]

A completion request will still have to wait for a spot in the queue - which can be running on all cylinders. So even if it's the highest priority it will have to wait for an opening to appear.

It's also true with this PR: unless the limit is Infinity, the request will need to wait in the queue until there's a spot available – albeit it's far more likely that there will be spot available most of the time since it's a separate queue, but if you want the request to fire immediately, we should not use a queue at all.
Like @Murderlon said, it's probably OK if the request has to wait for a bit, it's a necessary tradeoff IMO.

[schonert]

but that doesn't matter, right?

Well, it matters if you're aiming for a snappy upload feeling while uploading tons of large files. In our case we need to generate thumbnails and smaller versions of large videos. The faster we can get the GO signal from the uploader the better.

Having two files idling at 100% seems unnecessary in my opinion.
I understand the priority of simplicity and documentation. I'll finish up the PR and move on to patching this up for our use case.

I think we should instead change the priority of those requests, on the same queue.

What should the priority be? Infinity?

[schonert]

@aduh95 Reading through the code I noticed that if there are multiple handlers with Infinity the last one added would be the most important.
Since we won't find anything greater than infinite, the index will always be -1.

uppy/packages/@uppy/utils/src/RateLimitedQueue.js

Lines 106 to 113 in 169de84

	const index = this.#queuedHandlers.findIndex((other) => {
	return handler.priority > other.priority
	})
	if (index === -1) {
	this.#queuedHandlers.push(handler)
	} else {
	this.#queuedHandlers.splice(index, 0, handler)
	}

We could set abortMultipartUpload and createMultipartUpload to priority: Infinity and change uploadPartBytes to something like priority: 99999999. This should ensure completion requests are served first?

Also, should we consider a first-in-first-served approach?
Changing line 107 to return handler.priority >= other.priority should do the trick.

[schonert]

@aduh95 okay now I doubt my ability to comprehend the queue logic 😅 Disregard my last comment. It's already first-in-first-servered. So setting them to Infinity would do the trick.

[Murderlon]

Thanks for the changes. @schonert how have you tested this to confirm it works for you?

[aduh95]

We could set abortMultipartUpload and createMultipartUpload to priority: Infinity and change uploadPartBytes to something like priority: 99999999. This should ensure completion requests are served first?

uploadPartBytes priority is set to Infinity, because once the signature has been generated, we want the upload to start right away to avoid the risk of the signature expiring – which is arguably worst than having an upload stuck at 100% for a few minutes. However, because fetchSignature priority is the default 0, and uploadPartBytes depends on it, any request added to the queue a request with priority >0 would still execute first.

What should the priority be? Infinity?

So because they do not expire, contrary to uploadPartBytes, using priority Infinity for completion and abort requests is not the best, maybe priority 1 would suffice – it would still behave the same as Infinity (they need to wait for on going requests to complete, then start right away (until we introduce requests with priority 2 or something, but I don't see why we would do that)), and it would decrease the risk of clients on slow connections getting Request has expired errors.

[schonert]

Thanks for the changes. @schonert how have you tested this to confirm it works for you?

@Murderlon this is identical to my previous monkey patched solution. Works as expected. Afraid it will not solve the race-conditions I initially set out to solve.

@aduh95 makes sense. Updated the pull request 👍

[aduh95]

Afraid it will not solve the race-conditions I initially set out to solve.

I wouldn't call that a race condition, but I get where you coming from. I'm afraid we would need to come up with a completely new queuing model if we wanted to address that. In most cases, the only requests that need to be limited are the uploadPartBytes and therefor fetchSignature; all the other requests are likely HTTP/2 anyway, so limiting them does not do much good, and could skip the queue altogether; I'm afraid we couldn't have it as the default behavior though, as it could be quite breaking for folks who are using a backend using HTTP/1.1 (limited to 6 concurrent connexion to the same domain).

Note that in most cases, chunks are 5MiB large, so you can expect it will take at most the time for your client to upload 5MiB before the complete/abort request is actually sent.

[Murderlon]

Thanks!

[schonert]

I'm afraid we couldn't have it as the default behavior though

Agree - and I see the headache of documenting how a limit of 5 is not actually a limit of 5.

A way to modify this behavior would have been great tho. Extending the HTTPCommunicationQueue and replacing companionCommunicationQueue required quite a few dirty tricks.

Note that in most cases, chunks are 5MiB large, so you can expect it will take at most the time for your client to upload 5MiB before the complete/abort request is actually sent.

We've gone ahead and increased our chunks to between 50-100mb - depending on the actual size and type of the file. We found an increase in performance with the increased chunk size while calculating the checksum and uploading.