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

make module context-aware #1394

Open
mac-g opened this issue Mar 23, 2019 · 42 comments · Fixed by #2235
Open

make module context-aware #1394

mac-g opened this issue Mar 23, 2019 · 42 comments · Fixed by #2235

Comments

@mac-g
Copy link

mac-g commented Mar 23, 2019

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:

Error: Module did not self-register.
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:840:18)
    at Module.load (internal/modules/cjs/loader.js:666:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:606:12)
    at Function.Module._load (internal/modules/cjs/loader.js:598:3)
    at Module.require (internal/modules/cjs/loader.js:705:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at Object.<anonymous> (~/node_modules/canvas/lib/bindings.js:3:18)
    at Module._compile (internal/modules/cjs/loader.js:799:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:810:10)
    at Module.load (internal/modules/cjs/loader.js:666:32)

Steps to Reproduce

In main thread...

const {Worker} = require('worker_threads');
var worker1 = new Worker('./child-thread.js'); 
var worker2 = new Worker('./child-thread.js');
// etc.

In child-thread.js...

var Canvas = require('canvas');
var canvas = Canvas.createCanvas(200, 200);
var ctx = canvas.getContext('2d');
// etc.

References

Node.js documentation:
https://nodejs.org/api/addons.html#addons_context_aware_addons
Similar issue:
schroffl/node-lzo#11

Environment

  • node-canvas version 2.4.1
  • Mac OS 10.14.3
  • node version 11.12.1
@LinusU
Copy link
Collaborator

LinusU commented Apr 1, 2019

PRs welcome 👍

@jgoralcz
Copy link

jgoralcz commented Jun 1, 2019

This would be an incredible addition to node-canvas. Not sure where to fully dive in and modify the code though 🤔

@ruiwei

This comment was marked as duplicate.

@anshul-kai
Copy link

Nope. Need to fork processes and can't use worker threads.

@siddarthareddyt
Copy link

#1409 doesn't seem to fix this issue. Any plans to provide support for worker_threads?

@DePasqualeOrg
Copy link

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

@mistval

This comment was marked as duplicate.

@lateiskate

This comment was marked as duplicate.

@anna-rmrf

This comment was marked as duplicate.

@mac-g
Copy link
Author

mac-g commented Jul 4, 2020

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.

@lostrepo
Copy link

lostrepo commented Jul 5, 2020

Maybe this will help.

I'm total noob in C++
But I use something like that to use h264 encoder in worker_threads

#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();
}

@maxreisner
Copy link

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!

@Mjtlittle
Copy link

Mjtlittle commented Sep 7, 2020

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.

schroffl/node-lzo#11

specifically this replacement...

NODE_MODULE_INIT(/* exports, module, context */) {
    Init(exports, context);
}

@enricodeleo

This comment has been minimized.

2 similar comments
@dapriett

This comment has been minimized.

@lovanwubing

This comment has been minimized.

@dolcalmi
Copy link

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.

schroffl/node-lzo#11

specifically this replacement...

NODE_MODULE_INIT(/* exports, module, context */) {
    Init(exports, context);
}

It works... I had to recompile it with the next change to make it work with worker threads

// NODE_MODULE(canvas, init);
NODE_MODULE_INIT() {
    init(exports);
}

@enricodeleo
Copy link

@dolcalmi @Mjtlittle can you please submit a pull request? I think we all would take advantage of workers support

@danielyaa5
Copy link

danielyaa5 commented Oct 31, 2020

I tried this actually and I dont think its enough.

After I compile and run it in worker thread I get this error

TypeError: Canvas expected

I'm trying to understand the proper way to do this but I have no idea

https://nodejs.org/api/addons.html

@dteh
Copy link

dteh commented Nov 2, 2020

@danielyaa5 I can confirm recompiling with the NODE_MODULE_INIT macro worked for me. canvas.createCanvas(200,200) returned [Canvas 200x200].

@yeldarby
Copy link

yeldarby commented Nov 9, 2020

@dolcalmi or @dteh -- is a fork that has this change applied available somewhere?

Edit: yes, installed @dteh's fork from source and it worked!

brew install pkg-config cairo pango libpng jpeg giflib librsvg
npm install https://github.com/dteh/node-canvas#ISSUE-1394 --build-from-source

@effen1337
Copy link

effen1337 commented Nov 11, 2020

@yeldarby I used @dteh's fork and I actually get Error [TypeError]: Canvas expected when I use a worker.

EDIT: and even FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope

@dteh
Copy link

dteh commented Nov 11, 2020

@effen1337 perhaps it is a platform issue? I've successfully built and deployed it on OSX 10.15.6 and CentOS 7.8.2003

@phcreery
Copy link

Workaround as been depreciated in Electron 14. electron/electron#18397

@niyan-ly
Copy link

niyan-ly commented Jan 28, 2022

Node.js worker will share memory.
Native addon may run in different context, but share the same memory.
The canvas expected error raise because class Canvas create a new constructor template ctor each time the addon was loaded, and bind this template to its static member constructor. Then class Context2d try to find its instance on New. But since workers are sharing memory, only worker who load canvas.node lastly will work as expected(FILO like).

And that's what context-aware means, workers run in different context, but share memory.

image

@LosTigeros
Copy link

LosTigeros commented Jan 28, 2022

If you're using it on a backend I've found and currently using alternative way.
Basically I'm using BullMQ queue with Sandboxed processes to run multiple node-canvas instances. It works like a charm and was exactly what I needed.

@SonahtQ
Copy link

SonahtQ commented Mar 30, 2022

Any progress on that? or working workaround?

@SBRK

This comment was marked as duplicate.

@nateGarcia

This comment was marked as duplicate.

@zbjornson
Copy link
Collaborator

#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!

@kylerchin

This comment was marked as spam.

@DrMeepso
Copy link

Any updates?

@Balearica
Copy link

@zbjornson It looks like this issue was accidentally closed as completed due to the following sentence in #2235: We're closer to being able to close #1394. It looks like the issue is still outstanding and should be re-opened.

@chearon chearon reopened this Nov 26, 2023
@BowgartField
Copy link

any updates ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet