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

src: add buildflag to force context-aware addons #29631

Closed

Conversation

@codebytere
Copy link
Member

codebytere commented Sep 20, 2019

This PR adds a new cli flag --force-context-aware whose purpose is to prevent usage of native node addons that aren't context aware. In an embedder context, loading native addons ahas always been challenging due to the need to rebuild, to link to the right node.lib/dll, and to ensure the NODE_MODULE_VERSION is correct.

Since Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts, this creates a host of problems for multi-process framework like Electron.

This new flag would allow Electron and other embedders to enforce restrictions more cleanly and prevent unexpected behavior.

More context for embedders can be found here: #18397

cc @MarshallOfSound

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch 2 times, most recently from aaa73db to 469bdbe Sep 20, 2019
doc/api/cli.md Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch from 469bdbe to 23733d3 Sep 20, 2019
Copy link
Member

addaleax left a comment

This LGTM, although a test would be great.

Since Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts, this creates a host of problems for multi-process framework like Electron.

Just to make sure there’s no misunderstanding, this is the case for only non-context-aware addons, and that is the point of this PR, right?

src/env.cc Outdated Show resolved Hide resolved
src/node_binding.cc Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch from 23733d3 to 4d315c3 Sep 20, 2019
@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Sep 20, 2019

Yes indeed! Only the case for non-context-aware addons. I'll add a test as soon as i can :)

doc/api/errors.md Outdated Show resolved Hide resolved
src/node_binding.cc Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch 2 times, most recently from 6c5909f to 93d2420 Sep 21, 2019
@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Sep 21, 2019

@addaleax i think (?) my test should cover it, it's definitely not the prettiest test i've ever written so please let me know if you think there's a better pattern for testing this kind of cli flag!

@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch from 93d2420 to 93dc647 Sep 21, 2019
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch 3 times, most recently from 3ac2c4a to c5efc2a Sep 21, 2019
Copy link
Member

addaleax left a comment

Yeah, the test looks good to me too :)

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

bnoordhuis left a comment

A test that checks loading a context-aware add-on works would be nice too. You can co-opt one of the other context-aware add-on tests if you like, e.g.:

diff --git a/test/addons/zlib-binding/test.js b/test/addons/zlib-binding/test.js
index b50817aa5d..7a488c9a3b 100644
--- a/test/addons/zlib-binding/test.js
+++ b/test/addons/zlib-binding/test.js
@@ -1,3 +1,4 @@
+// Flags: --force-context-aware
 'use strict';
 
 const common = require('../../common');
doc/api/cli.md Outdated Show resolved Hide resolved
test/addons/force-context-aware/binding.cc Outdated Show resolved Hide resolved
test/addons/force-context-aware/test.js Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch from c5efc2a to 094d888 Sep 23, 2019
@Trott Trott added the author ready label Sep 24, 2019
@nodejs-github-bot

This comment has been minimized.

@codebytere codebytere force-pushed the codebytere:warn-prevent-context-aware branch from 094d888 to bcef054 Sep 24, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Sep 25, 2019
PR-URL: #29631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Sep 25, 2019
PR-URL: #29631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Sep 25, 2019

Landed in 5058c7f...f120e6d

@Trott Trott closed this Sep 25, 2019
@codebytere codebytere deleted the codebytere:warn-prevent-context-aware branch Sep 25, 2019
<a id="ERR_NON_CONTEXT_AWARE_DISABLED"></a>
### ERR_NON_CONTEXT_AWARE_DISABLED

A non-context-aware native addon was loaded in a process that disallows them.

This comment has been minimized.

Copy link
@jasnell

jasnell Sep 25, 2019

Member

including a link here for context-aware like in the cli.md file would be helpful

@@ -393,6 +393,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"silence all process warnings",
&EnvironmentOptions::no_warnings,
kAllowedInEnvironment);
AddOption("--force-context-aware",
"disable loading non-context-aware addons",

This comment has been minimized.

Copy link
@jasnell

jasnell Sep 25, 2019

Member

extremely minor take it or leave it nit.. but the double negative here feels odd ... "disable ... non" ... perhaps "allow only context-aware addons" as an alternative?

BridgeAR added a commit that referenced this pull request Oct 9, 2019
PR-URL: #29631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR added a commit that referenced this pull request Oct 9, 2019
PR-URL: #29631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: TBD
@BridgeAR BridgeAR referenced this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.