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

Partially closes #13 on Firefox in order to prepare to Chrome future release #261

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

gwdp
Copy link
Contributor

@gwdp gwdp commented Jan 15, 2022

Since #13 was reported, Chrome had an issue where

cancel () {
would not be fired, and I believe, because of that nothing was ever implemented on this handler although it was always called by Firefox.

Also, reported on #13, ticket was filled in for the chrome team (ticket #638494) a few years ago and it got merged last week.

Testing on Canary version 99.0.4828.0, cancellation event still not being triggered but worker seems to be killed on user cancellation causing stream saver to write to throw at write(). (more investigation on this behaviour is actually needed but it does not allow more writing on the main thread as expected).

Now, to make sure we do abort the operation at user cancellation on Firefox, that does not kill the worker but just closes the file descriptor and maintains the worker running without sending any signal to the main thread the proposed changes are implemented.

@steveseguin
Copy link

This also resolved a rather new issues I was facing with Chrome 98 and StreamSaver.js. Thank you for sharing the patch.

@gwdp
Copy link
Contributor Author

gwdp commented Feb 11, 2022

@jimmywarting Can we get a merge on it? 🥺

@jimmywarting
Copy link
Owner

yea, sure. it's a quick and easy fix
i would rather want to rewrite the hole communication as discussed in #13 (comment)
but haven't had the time to do it...

also this needs to be backwards compatible also cuz it depends on a public service worker that everyone depends on.
so a rewrite would be tough... also i don't know how much love i want to put into it now that we have transferable streams and even https://github.com/whatwg/fs/ at our disposal

@jimmywarting jimmywarting merged commit 5297ec4 into jimmywarting:master Feb 11, 2022
yume-chan pushed a commit to yume-chan/StreamSaver.js that referenced this pull request Feb 18, 2022
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 <[email protected]>
Co-authored-by: Jimmy Wärting <[email protected]>
Co-authored-by: Kyle J. Davis <[email protected]>
Co-authored-by: Hein Haraldson Berg <[email protected]>
Co-authored-by: Bonjour Comosava <[email protected]>
Co-authored-by: Christopher Chamberlain <[email protected]>
Co-authored-by: tania daniela paiva <[email protected]>
Co-authored-by: Gabriel Pacheco <[email protected]>
Co-authored-by: Jeremy Kahn <[email protected]>
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.

3 participants