@uppy/aws-s3-multipart: fix crash on pause/resume

[g8y3e]

Fixes: #4648

This bug occurs when we attempt to press the pause/resume button after the first batch of chunks has been uploaded. When the second batch starts the upload process, we end up with an array of chunks in the multipart upload package that looks like this: [null, null, null, null, chunk, chunk] As a result, when we resume, we continue to attempt to retrieve data from the null chunks. This is why I added a check to skip them.

Before fix we have error:

[Uppy] [11:22:52] TypeError: Cannot read properties of null (reading 'getData')
    at index.js:333:31
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (asyncToGenerator.js:3:1)
    at _next (asyncToGenerator.js:25:1)
    at _ZoneDelegate.invoke (zone.js:372:26)
    at Object.onInvoke (core.mjs:26291:33)
    at _ZoneDelegate.invoke (zone.js:371:52)
    at Zone.run (zone.js:134:43)
    at zone.js:1275:36
    at _ZoneDelegate.invokeTask (zone.js:406:31)

After fix - resume successfull

[Murderlon]

Thanks for the PR. There is one issue in CI:

/home/runner/work/uppy/uppy/packages/@uppy/aws-s3-multipart/src/index.js
Error:   400:9  error  Async method 'uploadChunk' expected no return value  consistent-return

Can you take a look?

[aduh95]

Thanks for the PR, I don't think this is a correct fix, there should not be any null chunks to begin with, and returning undefined is going to create more TypeErrors later.
For some reason I'm not able to reproduce the reported error, can you tell more about the setup required to reproduce? Any chance you're able to reproduce from the yarn dev workflow on this repo?

[g8y3e]

Thanks for the PR, I don't think this is a correct fix, there should not be any null chunks to begin with, and returning undefined is going to create more TypeErrors later. For some reason I'm not able to reproduce the reported error, can you tell more about the setup required to reproduce? Any chance you're able to reproduce from the yarn dev workflow on this repo?

To reproduce it you need upload file that huge enough - to have multiple chunks count- that cannot be loaded in one upload iteration - for example: I'm using file 100Mb - and wating when first batch of chunk will be loaded - something like this:

After I'm waiting when second batch of chunks will start uploads - and only then press pause button - and when you will press resume button you will see error from uppy

That actually points to this line of code:

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

Line 380 in e04ae8b

const chunkData = chunk.getData()

Thats why I added check for chunk == null - maybe better add break statemanet just to exit this infinite loop for this chunk

[g8y3e]

Tested with adding break on local environment now upload process resumes - without any error - and finishes successfully

[aduh95]

Thanks for the repro steps, somehow I'm still unable to reproduce the same error, I'm getting a different one, I'm sending a PR to fix it: #4748. I'll try to find ways to reproduce.

Tested with adding break on local environment now upload process resumes - without any error - and finishes successfully

Using break here has the same effect as return, in both cases the function returns undefined. I would prefer an approach where we don't call uploadChunk at all if there are no chunks to upload.

[aduh95]

To the best of my knowledge, chunk should never be null inside uploadChunk because of this check:

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

Lines 336 to 339 in 9ad48f8

	const alreadyUploadedInfo = alreadyUploadedParts.find(({ PartNumber }) => PartNumber === partNumber)
	if (alreadyUploadedInfo == null) {
	return this.uploadChunk(file, partNumber, chunk, signal)
	}

The fact that you are seeing an error means that it hasn't been uploaded (or at least that the part number doesn't match), and yet the chunk has been removed from the memory 🤔 There's definitely something wrong, and I'm afraid the fix you are suggesting will not fix it, only make the error disappear.
Can you try to see with a debugger what's the value of alreadyUploadedParts?

[g8y3e]

To the best of my knowledge, chunk should never be null inside uploadChunk because of this check:

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

Lines 336 to 339 in 9ad48f8

	const alreadyUploadedInfo = alreadyUploadedParts.find(({ PartNumber }) => PartNumber === partNumber)
	if (alreadyUploadedInfo == null) {
	return this.uploadChunk(file, partNumber, chunk, signal)
	}

The fact that you are seeing an error means that it hasn't been uploaded (or at least that the part number doesn't match), and yet the chunk has been removed from the memory 🤔 There's definitely something wrong, and I'm afraid the fix you are suggesting will not fix it, only make the error disappear. Can you try to see with a debugger what's the value of alreadyUploadedParts?

I checked alreadyUploadedParts and it is empty - and because it gets information from listParts - means that issue in other part of code.
Sorry for troubles - I think we can close this PR

[aduh95]

OK, so you think the error doesn't come from Uppy code? If you're able to find the cause, and update the issue with some hints on how you fixed it, that'd be very helpful.
Closing as you suggested, let's continue the discussion on #4648.

[g8y3e]

OK, so you think the error doesn't come from Uppy code? If you're able to find the cause, and update the issue with some hints on how you fixed it, that'd be very helpful. Closing as you suggested, let's continue the discussion on #4648.

Yes. Error because of listParts. I will try search cause and inform in discussion.