Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
theazgra opened this issue Apr 9, 2024 · 9 comments · Fixed by #138
Closed

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

theazgra opened this issue Apr 9, 2024 · 9 comments · Fixed by #138

Comments

@theazgra
Copy link

theazgra commented Apr 9, 2024

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:

  • UpChunk v3.3.2
  • Safari 17.0 (19616.1.27.211.1)
  • Apple M1 with 8GB RAM
  • macOS 14.0 (23A344)
@daytime-em
Copy link

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
Copy link

karlcow commented Apr 12, 2024

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

@theazgra
Copy link
Author

theazgra commented Apr 12, 2024

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

I have came up with workaround for this bug. theazgra@1fcd4af

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

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

@cjpillsbury
Copy link
Contributor

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
Copy link
Contributor

#138 in case you wanted to follow along! hopefully we can get a release out soon

@theazgra
Copy link
Author

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
4 participants