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

worker: improve integration with native addons #23319

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax 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.

/cc @gabrielschulhof Who I remember had some suggestions for improving upon this approach

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels 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
@addaleax addaleax added addons Issues and PRs related to native addons. worker Issues and PRs related to Worker support. and removed node-api Issues and PRs related to the Node-API. labels Oct 7, 2018
@richardlau
Copy link
Member

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 addaleax added the wip Issues and PRs that are still a work in progress. label Oct 7, 2018
@addaleax
Copy link
Member Author

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
Copy link
Member

Trott commented Nov 20, 2018

@addaleax Still working on this?

@gabrielschulhof
Copy link
Contributor

@addaleax I don't believe we should be storing dlopen handles in an associative array where the key is the file name. We should just perform a dlopen() every time we need a module, and dlclose all the handles we've collected in an environment when we clean up the environment. The reason is that internally dlopen() already has an associative array where the key is the file name and thus it already caches the handles and increases a given handle's refcount if dlopen() is called again and the corresponding .so is already mapped into the process.

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)
Copy link
Contributor

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.

const re = /^Error: Module did not self-register\.$/;
assert.throws(() => require(`./build/${common.buildType}/binding`), re);
// This second `require()` call should not throw an error.
require(`./build/${common.buildType}/binding`);
Copy link
Contributor

@gabrielschulhof gabrielschulhof Dec 1, 2018

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
Copy link
Member

Trott commented Dec 4, 2018

Needs a rebase.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 7, 2018
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 7, 2018
This is an alternative to nodejs#23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.
@addaleax
Copy link
Member Author

This has been superseded by @gabrielschulhof’s #24861 :)

@addaleax addaleax closed this Dec 10, 2018
@addaleax addaleax deleted the worker-addons branch December 10, 2018 21:31
gabrielschulhof pushed a commit that referenced this pull request Dec 20, 2018
Originally from portions of #23319.

PR-URL: #24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gabrielschulhof pushed a commit that referenced this pull request Dec 20, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
Originally from portions of #23319.

PR-URL: #24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
Originally from portions of nodejs#23319.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
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]>
targos pushed a commit that referenced this pull request Jan 1, 2019
Originally from portions of #23319.

Backport-PR-URL: #25258
PR-URL: #24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
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]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Originally from portions of nodejs#23319.

PR-URL: nodejs#24861
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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]>
addaleax added a commit to addaleax/node that referenced this pull request Jan 22, 2019
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
danbev pushed a commit that referenced this pull request Jan 23, 2019
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]>
addaleax added a commit that referenced this pull request Jan 23, 2019
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]>
addaleax added a commit to addaleax/node that referenced this pull request Feb 17, 2019
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
addaleax added a commit to addaleax/node that referenced this pull request Feb 20, 2019
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
addaleax added a commit that referenced this pull request Feb 22, 2019
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: #26175
Fixes: #21481
Fixes: #21783
Fixes: #25662
Fixes: #20239
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
addaleax added a commit that referenced this pull request Feb 25, 2019
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: #26175
Fixes: #21481
Fixes: #21783
Fixes: #25662
Fixes: #20239
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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: #26175
Fixes: #21481
Fixes: #21783
Fixes: #25662
Fixes: #20239
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. wip Issues and PRs that are still a work in progress. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants