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

Add the possibility to terminate Web Workers #31

Merged
merged 1 commit into from Oct 14, 2015

Conversation

addaleax
Copy link
Collaborator

This adds a .terminateWorker method to the lzma object, which terminates the associated web worker (if web workers are available and have been used).

A possible (and my intended) use case is to use multiple LZMA() instances in parallel in order to increase overall compression speed; Since Web Workers, however, don’t seem to abide by the normal garbage collection rules, this requires some way of stopping them once their work is done.

This adds a `.terminateWorker` method to the lzma object
which, if appropiate, terminates the associated web worker.
nmrugg added a commit that referenced this pull request Oct 14, 2015
Added .terminateWorker() to be able to garbage collect the Web Worker.
@nmrugg nmrugg merged commit 14e044c into LZMA-JS:master Oct 14, 2015
@addaleax addaleax deleted the terminate-webworker branch October 14, 2015 15:33
@@ -2628,7 +2628,8 @@ var LZMA = (function () {
return {
/** xs */
compress: compress,
decompress: decompress
decompress: decompress,
terminateWorker: function() {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this was added to match lzma.js, but I'm wondering if maybe leaving it out is better since calling this terminateWorker() doesn't do anything. Perhaps it's better to just let it throw an error and let the coder check to see if it exists. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, I thought it might be good to have a consistent API across the platforms. I agree that it probably does not make a lot of difference, since this version of terminateWorker is only used when using Node.js or similar, where the code author will know for sure that there are no web workers involved.

I can’t really tell how much code using this module is expected to run both server-side and client-side; If you don’t think it’s a notable amount, you can just drop it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’ve done some thinking about this, and maybe it would be even better to have some way of accessing the web worker object itself (e.g. LZMA().worker(), which would return null/undefined/… in case there is no worker); Then, users could call .terminate() themselves. This might be a little more intuitive and would have the advantage of providing access to other parts of the Web Worker API (and especially future additions to it).

Copy link
Collaborator

@nmrugg nmrugg Oct 16, 2015 via email

Choose a reason for hiding this comment

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

addaleax added a commit to addaleax/LZMA-JS that referenced this pull request Oct 16, 2015
addaleax added a commit to addaleax/LZMA-JS that referenced this pull request Oct 16, 2015
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.

None yet

2 participants