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 worker env.[aka: context aware] #1981

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

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

#1394

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants