TulipCharts / tulipnode

Tulip Node is the official node.js wrapper for Tulip Indicators. It provides over 100 technical analysis overlay and indicator functions.
https://tulipindicators.org
GNU Lesser General Public License v3.0
497 stars 86 forks source link

Support for worker_threads #59

Open femanzo opened 5 years ago

femanzo commented 5 years ago

Hi, I was getting an error (Module did not self-register) when while running this package inside a Worker thread, I have fixed it by changing:

NODE_MODULE(tulind, Init)

to

NODE_MODULE_INIT()
{
  Init(exports);
}

on the tulind.cpp file, then rebuilding with node-pre-gyp install --build-from-source

I'm still testing, but it seems to be working fine. I didn't make a pull request because I don't want to mess up your code with my linters, but It would be nice to have this change on the main repo, in case anybody is having the same issue.

jeremyjs commented 5 years ago

After taking a look at the docs, it actually looks like there are a few other additions needed to properly support context awareness: https://nodejs.org/api/addons.html#addons_context_aware_addons and worker threads: https://nodejs.org/api/addons.html#addons_worker_support.

Tests would need to be added to ensure cleanup occurs and that no objects are accessed across threads. "...any global static data stored in the addon must be properly protected, and must not contain any persistent references to JavaScript objects."

I don't have the bandwidth to add this now, but I'd really like to use worker threads for my use case, so I might revisit this at a later time if no one else picks it up.

For now, it does seem like your patch allows the library to be used inside workers (I was able to use it successfully), but could possibly cause memory leaks or crashes due to cross-thread object usage.