search

jimmywarting / StreamSaver.js

StreamSaver writes stream to the filesystem directly asynchronous
https://jimmywarting.github.io/StreamSaver.js/example.html
MIT License
3.95k stars 413 forks source link

fix: Support large streams in Firefox #305

Closed jeremyckahn closed 1 year ago

jeremyckahn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jeremyckahn commented 1 year ago

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 commented 1 year ago

fyi, this is the root cause: https://github.com/w3c/ServiceWorker/issues/882#issuecomment-262754779 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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