search

muxinc / upchunk

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

Upload of large files 4GB+ fails on Safari. UpChunk or WebKit problem? #134

Closed theazgra closed 2 months ago

theazgra commented 3 months ago

Hi,

we have noticed a problem, while uploading large video files, of size 4GB+ on MacOS Safari. I am not sure, if anything can be done in UpChunk library, or if this is solely WebKit problem.

The UpChunk dispatches 'success' event right after the upload is started, that is because the very first call to ChunkedStreamIterable::read() fails, resulting in completed (done) upload.

The read call fails, because of an I/O error (NotReadableError)

image

Before attempting the upload, top reports only about 100MB of free RAM, if that can mean anything.

If I truncate the file to about 3GB, the upload work.

Do you have any experience with this?

Specification:

daytime-em commented 3 months ago

Hi!

Thank you for your report and detailed feedback. I'll pass this along and we'll get back to you with more info soon.

karlcow commented 3 months ago

It might be worth to open a reduced test case on https://bugs.webkit.org/

theazgra commented 3 months ago

@karlcow I will do!

I have created small repro, which demonstrates, that it actually is WebKit problem.

Edit: webkit bug report: https://bugs.webkit.org/show_bug.cgi?id=272600

<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>WebKit file stream reader</title>
</head>

<body>
    <input id="picker" type="file" />
    <label for="alternativeCheck">Use file slice</label>
    <input type="checkbox" name="alternative" id="alternativeCheck">

    <script>
        const picker = document.getElementById('picker');
        const checkbox = document.getElementById('alternativeCheck');

        picker.onchange = async () => {
            const file = picker.files[0];

            if (checkbox.checked) {
                // This method works
                const {done, value} = await file.slice(0, 65_536).stream().getReader().read();
                console.log(done);
                console.log(value);
            }
            else {
                // I/O operation failed
                const fileStream = file.stream();
                const fileStreamReader = fileStream.getReader();
                const {done, value} = await fileStreamReader.read()

                console.log(done);
                console.log(value);
            }
        };

    </script>
</body>
</html>
theazgra commented 3 months ago

@daytime-em I imagine, this must affect more users. Well all users using newest UpChunk versions on Safari. Will you try to find different solution, around the broken API, or will you wait for Safari team?

cjpillsbury commented 3 months ago

hey @theazgra! Time permitting, we're hoping to provide an "opt in" fallback to something similar to our old implementation. Unfortunately, that solution means that those 4GB+ files will have to be entirely loaded into memory (due to the nature of the WebKit bug), which isn't great either. In the interim, I just cut a release (https://www.npmjs.com/package/@mux/upchunk/v/3.3.3) that will at least give you an "error" with some info that you can display to users as a stop gap if that's helpful:

this.dispatch('error', { message: `Unable to read file of size ${this.file.size} bytes. Try loading from another browser.` });
theazgra commented 2 months ago

hey @cjpillsbury , thanks for the reply and new release!

I have came up with workaround for this bug. https://github.com/theazgra/forendors-upchunk/commit/1fcd4afbe861eb52a2fe7b80251c5a9d94c24e2d

Would this, if polished, be worth of PR to MuxChunk?

Currently we, on our side, will be using useFileSlicing when detecting Safari.

cjpillsbury commented 2 months ago

Hey @theazgra glad you were able to find a workaround! I always love contributions from folks and definitely appreciate the offer, but it may be hard to work through and communicate the details of a solution on this one within the time frame we're aiming for. That said, if I don't get time to implement the solution within the next few weeks (hoping to peel away some time starting today), I'd love to work with you on a mergeable solution!

Here's the short version of what I'll be doing (at least broad strokes. Things may change as I work through the actual implementation).:

  1. always attempt to use readable streams from files first
  2. provide an "opt in" config variable for falling back to the old method for reading files
  3. if/when an error occurs when attempting to read from the readable stream (detecting this is now implemented), fall back to the old method, aka reading the entire file into memory and slicing, similar to your implementation and also similar to the pre 3.x implementation
    1. This will be a completely different iterable implementation (might be a subclass though? I'll need to work through the details) - This is so we can easily isolate and remove the fallback if/when WebKit/Safari fixes the bug (🤞), but also to leave room for some future possible features to upchunk, like uploading directly from screen recordings instead of waiting until the recording is completed (also 🤞).
cjpillsbury commented 2 months ago

https://github.com/muxinc/upchunk/pull/138 in case you wanted to follow along! hopefully we can get a release out soon

theazgra commented 2 months ago

Hey @cjpillsbury thanks for headsup. Looking forward to the next release, will switch to it when available. Thanks for the quick replies and fast problem solving.

I will close this issue as it is being solved in #138