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

Support context-aware #89

Closed
wants to merge 5 commits into from
Closed

Support context-aware #89

wants to merge 5 commits into from

Conversation

jc-lab
Copy link

@jc-lab jc-lab commented Dec 30, 2019

No description provided.

Copy link
Owner

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Hi @jc-lab, and thanks for the PR!

Unfortunately, this addon is not context-aware in its current state, and marking it as such would be misleading.

At the very least, the static global constructor variables are incompatible with context-awareness, if you want to look into fixing that.

@jc-lab
Copy link
Author

jc-lab commented Dec 31, 2019

Hi, @addaleax Thanks for the comment.

I added a commit to solve the static constructors.

Please review.

I hope to be able to use lzma-native on the thread.

Thanks!

@addaleax
Copy link
Owner

@jc-lab Thanks! I think it might be easiest to remove the constructor references, though, and just disable calls without new in those cases.

If you do pursue the current route, you’ll also need to use a cleanup hook (AddCleanupHook, sadly Nan doesn’t provide a wrapper for that) that deletes the AddonContext so that there is no memory leak when loading the addon more than once.

I’ve also rebased #72 – switching to N-API altogether would be the ideal solution here, imo. When I originally tried, I had some issues with getting CI to pass, but I feel like there might be a chance some fixes in upstream Node.js have resolved those.

@jc-lab
Copy link
Author

jc-lab commented Dec 31, 2019

@addaleax

I also think N-API conversion is a good idea.

I looking forward to the N-API version soon!

@addaleax
Copy link
Owner

Okay, published v6.0.0 with N-API and thus with context-awareness – let me know how that works for you! :)

I’ll be closing this as there shouldn’t be anything more to do in order to support worker threads.

(Happy new year btw!)

@addaleax addaleax closed this Dec 31, 2019
@jc-lab
Copy link
Author

jc-lab commented Dec 31, 2019

@addaleax

Working good! Thank you.

Happy new year :)

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.

2 participants