-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
make module context-aware #1394
Comments
PRs welcome 👍 |
This would be an incredible addition to node-canvas. Not sure where to fully dive in and modify the code though 🤔 |
This comment was marked as duplicate.
This comment was marked as duplicate.
Nope. Need to fork processes and can't use worker threads. |
#1409 doesn't seem to fix this issue. Any plans to provide support for |
I'm having the same issue. I'd love to be able to use node-canvas in multiple worker threads to improve performance. Anyone want to tackle this? Seems like it should be straightforward to solve: https://github.com/nodejs/node/blob/master/doc/api/addons.md#context-aware-addons |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Looks like I'll have a chance to maybe take a stab at this in a week while I'm on leave. No promises. Has anyone else had a chance to take a look at it? If so, I'd be great to avoid hours of re-discovery. |
Maybe this will help. I'm total noob in C++ #include "v8.h"
#include <node.h>
#include <node_buffer.h>
using namespace v8; Per NodeJS instance addon data class AddonData {
public:
explicit AddonData(Isolate* isolate):
wh(0) {
// Ensure this per-addon-instance data is deleted at environment cleanup
node::AddEnvironmentCleanupHook(isolate, DeleteInstance, this);
}
// Per-addon data
int wh;
// ... omitted
// Node Buffer Object for YUV420 source
MaybeLocal<Object> ab;
// Node Buffer Object for h264 bitstream
MaybeLocal<Object> bsiab;
int createEncoder(){
int rv = WelsCreateSVCEncoder(&encoder_);
return encoder_ != NULL ? rv : 1;
}
void clean() {
if (encoder_) {
encoder_->Uninitialize();
WelsDestroySVCEncoder(encoder_);
}
}
static void abDeleteCb(char* data, void* hint) {
delete reinterpret_cast<MaybeLocal<Object>*>(hint);
}
static void noDeleteCb(char* data, void* hint) {
// do nothing
}
static void DeleteInstance(void* data) {
AddonData* ndata = reinterpret_cast<AddonData*>(data);
ndata->clean();
delete ndata;
}
}; Callback that accesses per NodeJS instance addon data void encode(const FunctionCallbackInfo<Value>& info) {
// Retrieve the per-addon-instance data
AddonData* data = reinterpret_cast<AddonData*>(info.Data().As<External>()->Value());
// encode YUV420 from buf
data->encode();
// construct h264 bitstream from bitstream info
data->parseBitstreamInfo();
data->bsiab = node::Buffer::New(info.GetIsolate(), (char*) data->bsi.data(), (size_t) data->bsiSize, AddonData::abDeleteCb, (void*) &data->bsiab);
info.GetReturnValue().Set(data->bsiab.ToLocalChecked());
} Per NodeJS instance module initializer (another method of initialization can be used too but this one was used in another addon that works with worker_threads and I'm total C++ noob) extern "C" NODE_MODULE_EXPORT void
NODE_MODULE_INITIALIZER(Local<Object> exports, Local<Value> module, Local<Context> context) {
Isolate* isolate = context->GetIsolate();
// Create a new instance of `AddonData` for this instance of the addon and
// tie its life cycle to that of the Node.js environment
AddonData* data = new AddonData(isolate);
// create encoder instance
data->createEncoder();
// Wrap the data in a `v8::External` so we can pass it to the method we expose
Local<External> external = External::New(isolate, data);
// ... omitted
exports->Set(
context,
String::NewFromUtf8(isolate, "encode", NewStringType::kNormal).ToLocalChecked(),
FunctionTemplate::New(isolate, encode, external)->GetFunction(context).ToLocalChecked()
).FromJust();
} |
I tried getting this to work with no success... If anyone who's more c++ oriented than me (and I'm not) would want some help fixing this, I'd be happy to help! |
I hope i'm not leading anyone astray, as I have no clue how the node API bindings work with creating a context-aware module; but I found a module called 'node-lzo' that has made changes to adapt their library, that I don't see present here. specifically this replacement... NODE_MODULE_INIT(/* exports, module, context */) {
Init(exports, context);
} |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It works... I had to recompile it with the next change to make it work with worker threads
|
@dolcalmi @Mjtlittle can you please submit a pull request? I think we all would take advantage of workers support |
I tried this actually and I dont think its enough. After I compile and run it in worker thread I get this error
I'm trying to understand the proper way to do this but I have no idea |
@danielyaa5 I can confirm recompiling with the |
@effen1337 perhaps it is a platform issue? I've successfully built and deployed it on OSX 10.15.6 and CentOS 7.8.2003 |
Workaround as been depreciated in Electron 14. electron/electron#18397 |
Node.js worker will share memory. And that's what |
If you're using it on a backend I've found and currently using alternative way. |
Any progress on that? or working workaround? |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
#1981 is open to address this but I haven't had time to review the recent changes. If anyone wants to try it out and/or review, that would help! |
This comment was marked as spam.
This comment was marked as spam.
It'll cause Vitest to fail, but we need it to update plans Automattic/node-canvas#1394
Any updates? |
@zbjornson It looks like this issue was accidentally closed as completed due to the following sentence in #2235: |
any updates ? |
Also just ran into the issue of Would love to see this happen, would really make my time a lot easier ;D |
any updates on this? |
As @anshul-kai pointed out above, the current workaround is to use Node.js From what I've tested, that way works fine. |
Issue or Feature
I've been experimenting with worker threads in node.js recently and noticed that the canvas module does not appear to be context-aware. In other words, the module will load/instantiate once, but as soon as another thread attempts to require canvas for it's own use, the following error is thrown:
Steps to Reproduce
In main thread...
In child-thread.js...
References
Node.js documentation:
https://nodejs.org/api/addons.html#addons_context_aware_addons
Similar issue:
schroffl/node-lzo#11
Environment
The text was updated successfully, but these errors were encountered: