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

fix: Support large streams in Firefox #305

Conversation

jeremyckahn
Copy link
Contributor

@jeremyckahn jeremyckahn commented Feb 24, 2023

Thank you for this excellent library! This change appears to fix #292 and #290. I tested it out by running the write-slowly.html example. In my experience with this change, the stream is kept open for longer than 30 seconds. It appears to complete writing data successfully regardless of how long the stream is open for.

@jimmywarting
Copy link
Owner

Oh, now i know the real reason why FF102 actually really started to fail all of the sudden.

v102 was the release when they started to support transferable readable streams.

I almost forgot how my hack did work. have to read all of the code and backlog of older issues/PRs everytime there is some kind of change that is needed.

The reason for why i don't do any keep alive logic (by pinging the service worker every now and then) in some certain circumstances is b/c when some stream is transferable, then the service worker dose not really have to listen to any post messages and pass the data along to the readable stream created inside of the service worker by createStream(port) inside of sw.js.

so when it supports transferable stream then it can just send the transferable readable stream (from the main thread) to the service worker and respond directly with that stream and not have to listen to any messages or doing any kind of work inside the service worker. meaning that you could essentially create a download stream and then immediately kill the service worker.
This works perfectly fine in chrome (it didn't need a keep alive logic) there was not any reason to ping it every x second.

but this dose not seem to be the case for Firefox...

@jimmywarting jimmywarting changed the title fix: [fixes #292] [fixes #290] Support large streams in Firefox fix: Support large streams in Firefox Feb 24, 2023
@jimmywarting jimmywarting merged commit 4bee517 into jimmywarting:master Feb 24, 2023
@jimmywarting
Copy link
Owner

Would be nice to not have to ping the service worker when it's chrome, but will let this slide for now.

@jeremyckahn jeremyckahn deleted the fix/support-large-streams-in-firefox branch February 25, 2023 16:50
@jeremyckahn
Copy link
Contributor Author

Thanks for the explanation @jimmywarting. That makes a lot of sense!

It seems reasonable to have the keepalive logic active in all environments. Its necessity appears to be dependent upon browser implementation details rather than any specification. So, targeting the lowest common denominator might result in the most predictable results and avoid bugs like #292.

I'm just happy to see this working again! 🙂

@jimmywarting
Copy link
Owner

jimmywarting commented Feb 25, 2023

fyi, this is the root cause: w3c/ServiceWorker#882 (comment) of why i deliberately did not do any keep alive when (as @jakearchibald put it) "respond with a ReadableStream where the service worker dose not need to do anything with it, can just be terminated and be pipe'ed in the background"

@jeremyckahn
Copy link
Contributor Author

Ah, got it. This is looking more like a Firefox bug, or at least undefined behavior at the spec level (currently). Bummer. 😕

Thanks for following up about this @jimmywarting!

@elliotsayes
Copy link

Hey, do you also have to update this line in StreamSaver.js?

mitm: 'https://jimmywarting.github.io/StreamSaver.js/mitm.html?version=2.0.0'

And I guess bump the package version + git tag

Also random question, am I able to host a copy of mitm.html elsewhere? I guess anywhere with HTTPS & text/html content-type should work, right?

@jimmywarting
Copy link
Owner

Hey, do you also have to update this line...

not necessary, but won't hurt. god way to go around the cache problem.
you can host the mitm on any site with https

jimmywarting added a commit that referenced this pull request Mar 6, 2023
* fix #172 firefox download only working once in HTTP (#173)

* - type checking (closes #181)
- better typing (jsdoc)

* fixed typos in readme (#196)

Just a few small grammatical fixes

* Clarify what ‘it’ references, fix typo/brain fart (#207)

* Fixed :) (#214)

* Fix minor spelling errors (#199)

* Fix typos

* fix minor spelling errors

* added funding

* Update FUNDING.yml

* Update README.md

* Update README.md

* Typo

* add licence

* Replicated chrome (canary:  99.0.4828.0) user cancellation behaviour on Firefox (#261)

Cleanup unused reference

* update link to whatwg/fs

* bump minor

* fix: support large streams in Firefox (#305)

* add note to self about FF v102

---------

Co-authored-by: Nomeji <nomeji@emersion.fr>
Co-authored-by: Jimmy Wärting <jimmy@warting.se>
Co-authored-by: Kyle J. Davis <halldirector@gmail.com>
Co-authored-by: Hein Haraldson Berg <hein@haraldsonberg.net>
Co-authored-by: Bonjour Comosava <IBILLIONAIRE@TUTANOTA.COM>
Co-authored-by: Christopher Chamberlain <chambca@gmail.com>
Co-authored-by: tania daniela paiva <taniadaniela1803@gmail.com>
Co-authored-by: Gabriel Pacheco <gabriel@totendev.com>
Co-authored-by: Jeremy Kahn <me@jeremyckahn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants