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

Fix native module loading in multi-context (--experimental-workers) #21517

Closed
wants to merge 2 commits into from

Conversation

rpetrich
Copy link
Contributor

@rpetrich rpetrich commented Jun 25, 2018

Fixes native module loading from multiple contexts by saving the module metadata provided by native modules when they call node_module_register. Also fixes a race condition when multiple contexts happen to load a module at the same time.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Maintain metadata on loaded native modules so that if additional
contexts load the same module, the metadata will be reused and the
registration function dispatched again.

Fix race condition where if multiple contexts load modules at the same
time, it's possible one of them could fail to load.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 25, 2018
Introduce a new addons/hello-world-from-worker test that loads and
calls a native module from the main thread, then dispatches a worker to
do the same. This ensures that a module is callable from multiple
threads/v8 isolates.
@rpetrich
Copy link
Contributor Author

rpetrich commented Jun 25, 2018

Not sure if further work should be done to cause dlopen followed by require of a native module to fail, or if addons/dlopen-ping-pong should be updated to match the new behaviour.

Also, it may be worthwhile to introduce new metadata in the node_module struct denoting whether or not a module is capable of being loaded in a alternate thread. Many native modules expect to call back into node from uv_default_loop, and this fails in a very confusing way when a native module is called from a worker.

@bnoordhuis
Copy link
Member

See #19731 (review). This PR has the same issue.

@rpetrich
Copy link
Contributor Author

Thanks, @bnoordhuis. Do you have any opinions on whether the well-known symbol is the recommended approach for multi-context aware modules? Unless all modules that use well-known are expected to be multi-context aware, it suffers from the same problem as you've described in #19731.

Should I split out the race condition fix into a separate PR? Also, perhaps it makes sense to raise an error if a multi-context unaware module is loaded in an alternate context? (though that would be a breaking change—as it is now, some of these modules can be used from a single worker, provided they don't reference the uv_default_loop in an unsafe way)

@bnoordhuis
Copy link
Member

Do you have any opinions on whether the well-known symbol is the recommended approach for multi-context aware modules?

I think it could be. It's new enough that we can simply say it should be multi-thread ready.

perhaps it makes sense to raise an error if a multi-context unaware module is loaded in an alternate context?

No strong opinion except that it's a breaking change. That's something to avoid unless there's a very clear benefit/cost ratio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants