@uppy/tus: pause all requests in response to server rate limiting

[aduh95]

When the remote server responds with HTTP 429, all requests are
paused for a while in the hope that it can resolve the rate limiting.
Failed requests are also now queued up after the retry delay. Before
that, they were simply scheduled which would sometimes end up
overflowing the limit option.

[Murderlon]

Can this be tested relatively easily in Jest? If so, would be great to have a test for it.

[aduh95]

Can this be tested relatively easily in Jest?

There are no "real" tests for @uppy/tus currently, I have the preconceived notion that writing network tests is tricky so I would tend to answer "no" 😅 We could try to reuse the test structure from tus-js-client, not sure how easy that would be though.

[Murderlon]

There are no "real" tests for @uppy/tus currently, I have the preconceived notion that writing network tests is tricky so I would tend to answer "no" 😅 We could try to reuse the test structure from tus-js-client, not sure how easy that would be though.

I see. If we could pass a callback to the plugin that is used for every request we could measure that, like I did for aws-s3-multipart:

uppy/packages/@uppy/aws-s3-multipart/src/index.test.js

Lines 168 to 218 in 87c716a

	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)
	})

But that's not the case and it's probably weird to add it just for testing purposes. E2E would also still be a bit challenging so I think we can skip it for now. But it is critical functionality that ideally should be tested in my opinion.

[aduh95]

E2E would also still be a bit challenging so I think we can skip it for now. But it is critical functionality that ideally should be tested in my opinion.

The hard truth has been spoken ☝️ Having a test suite that allows us to easily mock HTTP responses would be huge, and would let us write more interesting test cases.

[tim-kos]

Code lgtm to me overall, had some suggestions for naming and questions.
It's a pity that we cannot really have a test for this right now. we definitely need to make it happen. The complexity of the code is asking for bugs.

[tim-kos]

I'd expect retryAttempt to start with 1, as in the "first" attempt to retry. Also means if retryAttempt is 4, there were 4 retry attempts. Just reads more easily.

But options.retryDelays is a 0-indexed array. Might be useful to add a comment here that the first retry attempt means retryAttempt === 0 (if that is the case, ofc).

[aduh95]

retryAttempt starts at 0, because before on the first attempt, it's not a retry.
Looking at the https://github.com/tus/tus-js-client/blob/8574294c30765dbe3a71a7a066a88b060e0e84d6/lib/upload.js#L453, it looks like if we return false the retryAttempt will always be 0, so we need to hack our way around it..

[tim-kos]

What if this.opts.retryDelays was null, then this.#retryDelayIterator might be null and not be an iterator and thus have no next() method, causing a hard error. Should this.opts.retryDelays be set to [] if the consumer passes a useless value for it?

[tim-kos]

The naming suggests this to be an object (with a run method?), not a function, but I realize the pattern is used all over the file, so feel free to ignore me. This would be a big refactoring to keep it consistent in the file.

[mifi]

Does this PR also pause the requests towards Transloadit api2 (polling assembly json status) when they return 429? If not, then I believe that is also something that needs to be handled, because they share the same rate limiting counters in haproxy.

[aduh95]

Not sure if this is the most efficient way of doing it, but it seems to mostly do the trick for now.

[aduh95]

Hum I think I'm on to something here:

The RateLimitedQueue adjust its limit until it finds a value that doesn't trigger 429 errors. It seems to be working from what I can see.

[Murderlon]

Is the exponential backoff specifically implemented for transloadit and its HAProxy setup? Because we are implementing these changes in @uppy/tus. Will this work if people have their own tus server and don't upload to transloadit?

[aduh95]

I've made a separate PR for changes that I think are specific to Transloadit: #3414
I think the changes on this PR apply to any server: if the server responds with a 429, the client should back off imo, it's not limited to Transloadit.

[Murderlon]

nit: might be nice to write a tiny times(n, fn) function for these kinds of behaviors.

Addressed