Skip to content

Conversation

niyan-ly
Copy link

@niyan-ly niyan-ly commented Feb 2, 2022

This PR removes all function template, instead, keep trace of js constructor for each individual v8::Context. To achieve this, it holds all necessary js constructor under global property __node_canvas in each context. It may not that graceful, but is relatively safer than type casting many void* pointer.

@niyan-ly niyan-ly changed the title Support worker env.[aka: context aware] Support worker env.[aka: context aware#1394] Feb 2, 2022
@niyan-ly niyan-ly changed the title Support worker env.[aka: context aware#1394] Support worker env.[aka: context aware] Feb 2, 2022
@niyan-ly
Copy link
Author

niyan-ly commented Feb 2, 2022

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Thanks for starting on this! I don't think this is quite the right approach however. All static-duration data (such as font_face_list in Canvas.cc) has to be moved to instance data for the module, or has to not be JS objects and has to be thread-safe, which std::vector is not when there are multiple readers+writers. I think that is also the preferred approach for dealing with the Persistent handles, rather than storing them on a global object and retrieving them by calling into JS. https://nodejs.org/api/addons.html#context-aware-addons has the clearest info on this.

is relatively safer than type casting many void* pointer.

Which casts are you looking at? The "receiver" (in V8 terms) should be checked in all cases. For example:

> x = canvas.createCanvas(50,50) [Canvas 50x50] > y = x.getContext('2d') CanvasRenderingContext2D { canvas: [Canvas 50x50] } > z = y.createLinearGradient() CanvasGradient {} > z.addColorStop.call({}, 4, "abc") Uncaught TypeError: Illegal invocation 
@niyan-ly
Copy link
Author

niyan-ly commented Feb 4, 2022

Yes, you are right. I've just focused on existing issues, but didn't take a complete check on the package level. I will continue this work to see if we can make it.

@niyan-ly
Copy link
Author

niyan-ly commented Feb 4, 2022

I've movd all persistent handle and non v8 data into a new context level store AddonData, then passing it all around. And I am wondering how should we do the test work. It seems like I should run every existing test case in multiple worker env to coverage as much code as possible.

@niyan-ly
Copy link
Author

niyan-ly commented Feb 8, 2022

There are some extra works to do. Font registration are not at thread level, so move font_face_list into AddonData is actually a bug, it's a sharing property.

@niyan-ly
Copy link
Author

There are some extra works to do. Font registration are not at thread level, so move font_face_list into AddonData is actually a bug, it's a sharing property.

Fixed this issue by apply mutex lock, font_face_list are now shared in all threads(worker & main).

@nateGarcia
Copy link

Hello! Not sure how useful this will be... But, for my use case of parallelization in some visual-regression tests using Resemble & Codeceptjs, I am still receiving the same error

Module did not self-register: 'frontend/tests/codecept_e2e/node_modules/.pnpm/github.com+Automattic+node-canvas@d987d4666a32c4b8_190300f15912bbbd76c9d4ab8a89e735/node_modules/canvas/build/Release/canvas.node'. Error: Module did not self-register: 'frontend/tests/codecept_e2e/node_modules/.pnpm/github.com+Automattic+node-canvas@d987d4666a32c4b8_190300f15912bbbd76c9d4ab8a89e735/node_modules/canvas/build/Release/canvas.node'. at Object.Module._extensions..node (node:internal/modules/cjs/loader:1183:18) at Module.load (node:internal/modules/cjs/loader:975:32) at Function.Module._load (node:internal/modules/cjs/loader:822:12) at Module.require (node:internal/modules/cjs/loader:999:19) at require (node:internal/modules/cjs/helpers:102:18) at Object.<anonymous> (frontend/tests/codecept_e2e/node_modules/.pnpm/github.com+Automattic+node-canvas@d987d4666a32c4b8_190300f15912bbbd76c9d4ab8a89e735/node_modules/canvas/lib/bindings.js:3:18) at Module._compile (node:internal/modules/cjs/loader:1099:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10) at Module.load (node:internal/modules/cjs/loader:975:32) at Function.Module._load (node:internal/modules/cjs/loader:822:12) 
@niyan-ly
Copy link
Author

niyan-ly commented Apr 7, 2022

@nateGarcia Hi, did you still get this error on this PR? If so, could you give me a minimum replicable test case.

@MeemeeLab
Copy link

Hello, first of all, thanks for this pull request! it will help my project.
but I found the problem. This pr made it possible to load canvas in the worker, but it cannot be loaded multiple times.
here is the test case:

// index.js import {Worker} from 'worker_threads'; function createWorker() { new Worker(`./worker.js`); } createWorker(); createWorker();
// worker.js import {Canvas} from "canvas"; console.log('thread', Canvas.toString());

By loading worker multiple times, it crash by showing Module did not self-register.

It happens too when loading canvas from main thread, and worker thread:

// index.js import {Worker} from 'worker_threads'; import {Canvas} from "canvas"; console.log('main', Canvas.toString()); new Worker(`./worker.js`);
// worker.js import {Canvas} from "canvas"; console.log('thread', Canvas.toString());

Windows x64 21H2 22000.795

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

Labels

None yet

4 participants