Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: add opt-in fallback support (to roughly old impl) for readablestream errors. #138

Merged

Conversation

cjpillsbury
Copy link
Contributor

@cjpillsbury cjpillsbury commented Apr 22, 2024

This PR adds an "opt in" (via config options) fallback for cases where the (newer) ReadableStream File/Blob API fails (e.g. due to https://bugs.webkit.org/show_bug.cgi?id=272600). When "opted in", upchunk will revert to reading the file into memory, which is less ideal in general but may be preferred to simply failing and notifying (via an error event) about the failure. This allows developers to decide how they want to handle these cases.

The fallback can be enabled by setting the useFileSliceFallback options flag to true.

fixes #134

NOTE: The reason this isn't being turned on by default relates to the primary known error that necessitates the fallback. WebKit (including Safari) has a bug where, if a file is 4GB or larger, it will error when attempting to read the file using the File's ReadableStream. However, we migrated to using ReadableStream instead of loading the file into memory in v3.x specifically because of concerns/complaints that this could be very bad for JS thread heap memory consumption, sometimes resulting in crashes for memory-scarce environments or excessively large files. Since 4GB isn't an excessively small file (😅), and this is the case we are primarily accounting for, we didn't want to suddenly and automatically put developers back into that "greedy memory" usage scenario, but, since this issue has yet to be resolved in WebKit, we want to provide some answer to those cases beyond notifying an error (which we now do, too).

@cjpillsbury cjpillsbury requested a review from a team as a code owner April 22, 2024 20:50
@@ -150,6 +158,95 @@ export class ChunkedStreamIterable implements AsyncIterable<Blob> {
}
}

export class ChunkedFileIterable implements ChunkedIterable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: There's some amount of (fairly simple) redundancy between ChunkedFileIterable and ChunkedStreamIterable (i.e. almost everything that is not captured in the async generator/iterable function). We could create a common base class, but wasn't sure if that was a significant enough value add. Open to having pushback on this, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Said this in Slack, but I'm personally fine with this explicitly being a band-aid. We want to delete this code if Safari ever patches the issue, so I don't see any reason to spend more time than we have to on the fallback.

src/upchunk.ts Outdated
@@ -290,20 +390,46 @@ export class UpChunk {
this.success = false;
this.nextChunkRangeStart = 0;

if (options.useFileSliceFallback) {
Copy link
Contributor Author

@cjpillsbury cjpillsbury Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically monitors for an error event and then checks if there is also a corresponding error on the current instance of this.chunkedIterable. We could instead add concepts like error types/categories info to the event itself if this feels a bit too "acrobatics-y". Larger refactors to capture this logic in sendChunks() felt like the wrong path forward, especially if we want to treat this as a "stopgap" as much as possible.

Also, how do folks feel about useFileSliceFallback for a name? I'm amenable to changes here, since "Names Are Hard™️"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly calling it a workaround may help us/contributors remember why it's like this. How would you feel about useLargeFileWorkaround or something?

// Retry using ChunkedFileIterable, which reads the file into memory instead
// of a stream.
if (this.chunkedIterable.error) {
console.warn(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We "swallow the error" if we're falling back, but still log a warning for folks, just in case devs want some visibility here.

// Types appear to be getting confused in env setup, using the overloaded NodeJS Blob definition, which uses NodeJS.ReadableStream instead
// of the DOM type definitions. For definitions, See consumers.d.ts vs. lib.dom.d.ts. (CJP)
this.chunkedStreamIterable = new ChunkedStreamIterable(
this.chunkedIterable = new ChunkedStreamIterable(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went ahead and renamed these guys to be more generic, given the potential fallback.

@@ -268,6 +366,8 @@ export class UpChunk {
private eventTarget: EventTarget<Record<EventName, UpchunkEvent>>;

constructor(options: UpChunkOptions) {
this.eventTarget = new EventTarget();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this up top so it's ready to go for any potential code, including any refactors to constructor setup

this.dispatch('error', {
message: `Unable to read file of size ${this.file.size} bytes. Try loading from another browser.`,
});
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure we bail to not get unexpected success states and the like

Copy link

@daytime-em daytime-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a naming suggestion for useFileSliceFallback, no other feedback.

That's non-blocking imho so LGTM!

src/upchunk.ts Outdated
@@ -290,20 +390,46 @@ export class UpChunk {
this.success = false;
this.nextChunkRangeStart = 0;

if (options.useFileSliceFallback) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly calling it a workaround may help us/contributors remember why it's like this. How would you feel about useLargeFileWorkaround or something?

Copy link
Contributor

@mmcc mmcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE

@@ -150,6 +158,95 @@ export class ChunkedStreamIterable implements AsyncIterable<Blob> {
}
}

export class ChunkedFileIterable implements ChunkedIterable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Said this in Slack, but I'm personally fine with this explicitly being a band-aid. We want to delete this code if Safari ever patches the issue, so I don't see any reason to spend more time than we have to on the fallback.

@mmcc
Copy link
Contributor

mmcc commented Apr 22, 2024

Approved, but feels like we should update the documentation in this PR too? At the very least make sure it's included in the list of options.

@cjpillsbury
Copy link
Contributor Author

Approved, but feels like we should update the documentation in this PR too? At the very least make sure it's included in the list of options.

Yup good call! Will do before merging.

@cjpillsbury cjpillsbury merged commit 350112b into muxinc:master Apr 24, 2024
3 checks passed
@cjpillsbury
Copy link
Contributor Author

Went ahead and spruced up some other docs bits and bobs that I noticed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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