muxinc / upchunk

Uploads Chunks! Takes big files, splits them up, then uploads each one with care (and PUT requests).
MIT License
345 stars 46 forks source link

WebKit: in-memory FileReader failover, invalid Content-Range header. #145

Open theazgra opened 2 months ago

theazgra commented 2 months ago

Hi,

this could be considered as follow-up to #134 , which was resolved by #138 .

We have discovered new bug, affecting same users, so Safari users. We have detected the error while uploading file of size cca 1.5GB.

The implemented failover is used, as this messege is logged to the console

Unable to read file of size 1503686620 bytes via a ReadableStream. Falling back to in-memory FileReader!

Then the upload continues, failing with the last chunk. That is, because the Content-Range header is invalid. The upper limit of the range is larger than file size.

Headers of the last chunk upload:

Accept: */*
Content-Range: bytes 1384120320-1509949439/1509949439
Content-Type: video/mp4
Origin: https://www.forendors.cz
Referer: https://www.forendors.cz/
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site
User-Agent: Mozilla/5.0 (iPhone; CPU iPhone OS 17_3 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.3 Mobile/15E148 Safari/604.1

Note that: 1509949439 >1503686620

HTTP error from Google API:

XMLHttpRequest cannot load https://storage.googleapis.com/video-storage-gcp-us-east4-vop1-uploads/EuWRHwC3NSg6Uen2EQttnC?Expires=1722829957&GoogleAccessId=uploads-gcp-us-east1-vop1%40mux-video-production.iam.gserviceaccount.com&Signature=D%2FppImf0Tff85CxU3k%2B1cSrScqe5spcH0TGE6kN3Ki62T1IGCeoj6k5f6EdFf1RnDp0032%2BIiX8eiYV0EJ0yidex1eq1SsZf2hfCyBJ7X83iz3%2B1%2FLrAXwyNRPeRzuCnKuUU%2BrexQL5R%2BITA%2BlAZILDkRNg9mEA0crJ7IhoPsMe6bthxxWW0ZLDoC0vcsJBlhOyeoznoSLSH%2B%2FY3cloI2ZeLM4%2FumkI0YaGWHAg5ziAJR0Juqzu2EYP%2B037w2TXPqfrPVr%2Bka71QQ9DzFintSQkHm8Aqc%2FE1kGpD72%2FlN7bo76NDw3I4OcqmiXMVsQo4dWUVwSEo%2F%2BE8kcuXBJPhlg%3D%3D&upload_id=AHxI1nNfUtlwKC7BncAsAKE5JwqECfs48Ouzhdz3lzrlfwu-SON7K-5M0a3kdYMcPdt9wbZZdxFeELWYNxFHVzjxa5G8fNKYl2Wrb7kUVrE4CuG2Jg due to access control checks.

error handler is the correctly invoked atleast :)

Interestingly, second time uploading the same file, fixes the problem. (while using the same UpChunk instance, page is not reloaded)

theazgra commented 2 months ago

@cjpillsbury I will tag you directly, hope that is ok.

After looking at it more, the problem seems to be with nextChunkRangeStart property. Which is not reset correctly after fallback is applied.

nextChunkRangeStart is updated in successfulChunkUploadCb, then error occurs and readableStreamErrorCallback is called, nextChunkRangeStart is not reset-ed to 0.

This leads to nextChunkRangeStart being different in ChunkedFileIterable instance and UpChunk instance

ResettingnextChunkRangeStart in readableStreamErrorCallback seems to fix the problem.

if (options.useLargeFileWorkaround) {
      const readableStreamErrorCallback = (event: CustomEvent) => {
        // In this case, assume the error is a result of file reading via ReadableStream.
        // Retry using ChunkedFileIterable, which reads the file into memory instead
        // of a stream.
        if (this.chunkedIterable.error) {
          this.nextChunkRangeStart = 0; // <-- reset nextChunkRangeStart
          console.warn(
            `Unable to read file of size ${this.file.size} bytes via a ReadableStream. Falling back to in-memory FileReader!`
          );
          event.stopImmediatePropagation();

          // Re-set everything up with the fallback iterable and corresponding
          // iterator
          this.chunkedIterable = new ChunkedFileIterable(this.file, {
            ...options,
            defaultChunkSize: options.chunkSize,
          });
          this.chunkedIterator = this.chunkedIterable[Symbol.asyncIterator]();
          this.getEndpoint().then(() => {
            this.sendChunks();
          });
          this.off('error', readableStreamErrorCallback);
        }
      };
      this.on('error', readableStreamErrorCallback);
    }
cjpillsbury commented 2 months ago

Hey @theazgra sorry for the delayed response! That looks right to me and all makes sense. Feel free to issue a pull request if you wanted to contribute!