Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
worker: improve integration with native addons#23319
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addaleax commented Oct 7, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Oct 7, 2018
Native addons are now unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. Thanks go to Alexey Orlenko, Olivia Hugger and Stephen Belanger for reviewing the original version of this change. Refs: ayojs/ayo#118
richardlau commented Oct 7, 2018
Does this need updates to the docs? I'm assuming it only applies to context-aware addons: https://github.com/nodejs/node/blob/master/doc/api/addons.md#context-aware-addons |
addaleax commented Oct 7, 2018
@richardlau I’ve updated the documentation a bit (and added the WIP label here, this isn’t fully working yet) |
Trott commented Nov 20, 2018
@addaleax Still working on this? |
gabrielschulhof commented Nov 25, 2018
@addaleax I don't believe we should be storing Thus, I believe it is sufficient to add a cleanup hook when the environment is first created which simply closes all the handles which accumulate during the life cycle of that environment. If any of the addons add their own environment cleanup hooks, as advised in the documentation, then those will fire before the one that closes all the handles. So, I guess this also means that we should probably be storing the list of modules that were loaded into an environment on that environment so that the cleanup hook will find them when it fires. |
| node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); | ||
| } | ||
| NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be NODE_MODULE_CONTEXT_AWARE because Initialize makes use of the context.
| constre=/^Error:Moduledidnotself-register\.$/; | ||
| assert.throws(()=>require(`./build/${common.buildType}/binding`),re); | ||
| // This second `require()` call should not throw an error. | ||
| require(`./build/${common.buildType}/binding`); |
gabrielschulhofDec 1, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty strong change in behaviour. NODE_MODULE and even NODE_MODULE_CONTEXT_AWARE were so far expected to fail to register a second time around. Developers should explicitly move to modules that can be loaded multiple times by replacing those two macros with NODE_MODULE_INIT(/*exports, module, context*/){...} as per our docs, while ensuring that they address the concerns raised there.
If we require that modules move to NODE_MODULE_INIT then that's another reason we don't need to store dlopen handles in a file-name-keyed table.
Trott commented Dec 4, 2018
Needs a rebase. |
Originally from portions of nodejs#23319.
This is an alternative to nodejs#23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed.
addaleax commented Dec 10, 2018
This has been superseded by @gabrielschulhof’s #24861 :) |
Originally from portions of #23319. PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
This is an alternative to #23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Originally from portions of #23319. PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
This is an alternative to #23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Originally from portions of nodejs#23319. PR-URL: nodejs#24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
This is an alternative to nodejs#23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. PR-URL: nodejs#24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Originally from portions of #23319. Backport-PR-URL: #25258 PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
This is an alternative to #23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. Backport-PR-URL: #25258 PR-URL: #24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Originally from portions of nodejs#23319. PR-URL: nodejs#24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
This is an alternative to nodejs#23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. PR-URL: nodejs#24861 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Unloading native addons from the main thread was an (presumably unintended) significant breaking change, because addons may rely on their memory being available after an `Environment` exits. This patch only restricts this to Worker threads, at least for the time being, and thus matches the behaviour from nodejs#23319. Refs: nodejs#24861 Refs: nodejs#23319
Unloading native addons from the main thread was an (presumably unintended) significant breaking change, because addons may rely on their memory being available after an `Environment` exits. This patch only restricts this to Worker threads, at least for the time being, and thus matches the behaviour from #23319. PR-URL: #25577 Refs: #24861 Refs: #23319 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Unloading native addons from the main thread was an (presumably unintended) significant breaking change, because addons may rely on their memory being available after an `Environment` exits. This patch only restricts this to Worker threads, at least for the time being, and thus matches the behaviour from #23319. PR-URL: #25577 Refs: #24861 Refs: #23319 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: nodejs#23319
Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: nodejs#23319
Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: #23319 PR-URL: #26175Fixes: #21481Fixes: #21783Fixes: #25662Fixes: #20239 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: #23319 PR-URL: #26175Fixes: #21481Fixes: #21783Fixes: #25662Fixes: #20239 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: #23319 PR-URL: #26175Fixes: #21481Fixes: #21783Fixes: #25662Fixes: #20239 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Native addons are now unloaded if all Environments referring to it
have been cleaned up, except if it also loaded by the main Environment.
/cc @gabrielschulhof Who I remember had some suggestions for improving upon this approach
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes