@uppy/aws-s3-multipart: Fix race condition in #uploadParts

[mogzol]

The #uploadParts function calls itself after any part is uploaded. It also
determines which new chunks to upload based on their state.busy value. This
introduced a race condition, as state.busy was being set to false in the XHR
event handlers. So if one part were to complete while another part had finished
the XHR request, but not yet completed, then an upload for that second part
would be started again, despite the fact that the previous upload was still in
progress. Multiple uploads for the same part at the same time cause numerous
issues, and should never happen.

This is especially noticeable when an XHR request fails. #uploadPart is
wrapped in #retryable, so the part will be retried, however, for the entire
retryDelay, the chunk's state.busy value would be false, meaning that if
any other part completed, this part would be uploaded again, despite the fact
that the upload is already ongoing.

To fix this, this commit moves setting state.busy to the before and after
functions of the #retryable call, so a part will remain busy for the entire
time it is being uploaded/retried.

[mogzol]

Assuming this gets approved, what is the process for backporting a fix to the 2.x release? I need this fix in my application, which is still on uppy 2.x. Should I open a separate PR to the 2.x branch?

[arturi]

@mogzol Thanks for the PR! If approved, we’ll be able to cherry-pick and do a patch release for 2.x as well.

[Murderlon]

Thank you! This has been a bug I couldn't put my finger on where to find it for a while.

[Murderlon]

It would be nice to test this, but we don't really have the setup for it in e2e so I added a note for it in the PR. Unless you think it's best to test this in unit tests?

EDIT: I think we should be able to test this with nock and spying chunkState. We already have a test that does a lot of this:

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

Lines 42 to 119 in 88b08e3

	describe('without companionUrl (custom main functions)', () => {
	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, headers: { 1: { 'Content-MD5': 'foo' } } }
	}),
	})
	awsS3Multipart = core.getPlugin('AwsS3Multipart')
	})
	it('Calls the prepareUploadParts function totalChunks / limit times', async () => {
	const scope = nock(
	'https://bucket.s3.us-east-2.amazonaws.com',
	).defaultReplyHeaders({
	'access-control-allow-headers': '*',
	'access-control-allow-method': 'PUT',
	'access-control-allow-origin': '*',
	'access-control-expose-headers': 'ETag, Content-MD5',
	})
	// 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?partNumber=1'))
	.reply(function replyFn () {
	expect(this.req.headers['access-control-request-headers']).toEqual('Content-MD5')
	return [200, '']
	})
	scope
	.options((uri) => uri.includes('test/upload/multitest.dat?partNumber=2'))
	.reply(function replyFn () {
	expect(this.req.headers['access-control-request-headers']).toBeUndefined()
	return [200, '']
	})
	scope
	.put((uri) => uri.includes('test/upload/multitest.dat?partNumber=1'))
	.reply(200, '', { ETag: 'test1' })
	scope
	.put((uri) => uri.includes('test/upload/multitest.dat?partNumber=2'))
	.reply(200, '', { ETag: 'test2' })
	core.addFile({
	source: 'jest',
	name: 'multitest.dat',
	type: 'application/octet-stream',
	data: new File([new Uint8Array(fileSize)], {
	type: 'application/octet-stream',
	}),
	})
	await core.upload()
	expect(
	awsS3Multipart.opts.prepareUploadParts.mock.calls.length,
	).toEqual(1)
	scope.done()
	})

Would you be willing to try to make a test for it?

[mogzol]

I added a test to verify that busy doesn't get changed to false before done is changed to true. I don't know if that's really the best way to test this, but I couldn't figure out a better way. It does pass with my changes and fail without them.

EDIT: Oops I left a .only in the test, fixing that...

EDIT 2: Fixed

[Murderlon]

Thanks for all the effort!