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 public API to create isolate and context #20639

Closed
wants to merge 4 commits into from

Conversation

helloshuangzi
Copy link
Contributor

This change is used to support addon / embedding scenarios.
An addon/embedding scenario has requirement to create isolate / context with the same setting/tweak that is used in node main thread, instead of duplicating that part of logic by themself, which may lead to conflict in the future.

Please see this commit of napajs as an example.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 9, 2018
@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. embedding Issues and PRs related to embedding Node.js in another project. labels May 9, 2018
@addaleax
Copy link
Member

addaleax commented May 9, 2018

/cc @gireeshpunathil @cmfcmf

@jasnell
Copy link
Member

jasnell commented May 9, 2018

Ping @nodejs/n-api ... would it make sense to have n-api support for this?

@devsnek
Copy link
Member

devsnek commented May 9, 2018

i'm not quite sure how we would be able to represent contexts in napi but something like napi_create_env would probably work for isolates

@addaleax
Copy link
Member

addaleax commented May 9, 2018

@jasnell I think it’s going to make sense to work out and eventually provide an N-API-style embedder API for Node.js, yes. But this makes sense regardless, and we’d probably put more thoughts into a whole new API that we can design from scratch.

@gabrielschulhof
Copy link
Contributor

I was thinking something as simple as

napi_env napi_create_env();

... and then you can use napi_run_script() to launch something which, in turn, can require() native modules, for which the Node.js instance will use the special symbol if the modules don't self-register.

Now, if only we could figure out how not to leak the dlopen handles.

@gabrielschulhof
Copy link
Contributor

... or

napi_status napi_create_env(napi_env *result);

in keeping with our existing API look and feel.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the bug, I also have some misgivings about haphazardly exposing yet another piece of internal plumbing.

It doesn't result in a coherent public API but meanwhile makes maintenance harder.

src/node.cc Outdated
@@ -279,6 +279,8 @@ static v8::Isolate* node_isolate;

DebugOptions debug_options;

static ArrayBufferAllocator array_buffer_allocator;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't be static, it's tied to the isolate. That will be problematic because of the zeroFill field - that field is flipped on and off at run-time and that's observable (read: behaves erratically) between isolates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis zeroFill is tied with this condition if (auto zero_fill_field = env->isolate_data()->zero_fill_field()). Currently, the API of CreateIsolateData doesn't support parameter of zero_fill_field for embedding/addon users. It means zeroFill is only tied to the node main thread/environment. This is my understanding at first, and I thought it was not a bad design of node public API.
I can change it to let each isolate have its own ArrayBufferAllocator instance if you think the above understanding of the current node APIs doesn't work or just not right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helloshuangzi In that case, it might make sense to expose a way to create a Node.js-style ArrayBufferAllocator given an IsolateData* pointer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@helloshuangzi here is why it's not safe:

node/lib/buffer.js

Lines 115 to 122 in 46d335c

function createUnsafeArrayBuffer(size) {
zeroFill[0] = 0;
try {
return new ArrayBuffer(size);
} finally {
zeroFill[0] = 1;
}
}

That's the 'flipped on and off at run-time' I mentioned that is observable from another isolate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis , Yes, but 'zeroFill' is only available in node main isolate/environment because of if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) based on current node API.
I am thinking that it might be not a bad idea to make all isolates comply with node main isolate/environment about zero_fill_field。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's what I said all isolates will comply with node main isolate about setting of zero_fill_field, while only node main isolate can change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's what I said all isolates will comply with node main isolate about setting of zero_fill_field, while only node main isolate can change it.

I don’t think that’s a good idea. I agree with Ben, it’s not a good idea for one Isolate to be able to change the allocation mode of other Isolates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reinforce the point: what you'd see in other isolates is new arraybuffers randomly being filled with garbage instead of zeroes like they're supposed to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, @addaleax and @bnoordhuis, thanks for the feedback about this point. Let me change it to give an ArrayBufferAllocator instance for each isolate.
@addaleax, as you commented above, we will need:

  1. an API to create node.js-style ArrayBufferAllocator, and get its zero_fill_filed.
  2. a new API of CreateIsolateData which can take the parameter of zero_fill_field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, please help review!

@gabrielschulhof
Copy link
Contributor

@devsnek the napi_env is stored in a private field on the global object. We retrieve the global object from the context, which we receive during addon initialization from Node.js. Therefore napi_env is per-context.

@mhdawson
Copy link
Member

I share @bnoordhuis's concern for creating new public API without the complete picture. I also understand it may not be helpful to get in the way when it may take a while to come up with a complete API.

@helloshuangzi I'm wondering how urgent this is from your perspective? Is it significantly blocking your forward progress or something that is a nice to have in the short term?

@helloshuangzi
Copy link
Contributor Author

helloshuangzi commented May 10, 2018

@mhdawson and @bnoordhuis , thanks for your feedback.
About the coherence of node public API, given APIs like CreateIsolateData and CreateEnvironment are already there, I think NewIsolate and NewContext can be seen as a set of helpers for ease of use to complement those mentioned ones. It should not conflict with the future work to create a complete API.

@mhdawson, thanks for your asking about the urgency. We are working on re-implementing napajs, a node addon for js mutli-threading based on node public APIs (before we do most work by ourselves). Unfortunately, it is a blocker for this work.

@addaleax
Copy link
Member

Unfortunately, it is a blocker for this work.

@helloshuangzi Can you explain in what way it is a blocker? Generally, one should be able to get by using only the V8 APIs for creating Isolates + Contexts, so that seems like helpful information. (Like you said, the APIs introduced here would be “helpers”, and I am not sure in what way they are necessary themselves, tbh.)

@devsnek
Copy link
Member

devsnek commented May 10, 2018

@addaleax isolates that aren't tracked by node's platform tend to screwed over pretty badly through the lifetime. i think there's another pr to expose node's platform because of that.

@addaleax
Copy link
Member

@devsnek That’s not really related to this PR – CreateIsolateData() is the things which registers an isolate with the platform. The NewIsolate() API here does no such thing.

@helloshuangzi
Copy link
Contributor Author

@addaleax, I say it's a blocker because we are making every effort to maximize the consistency between napa worker and node default isolate/environment (or say it a node standard way to create isolate/context). The consistency is important to us to give napa users a smooth experience. That's why we are trying to implement napa based on node public API. Without the proposed APIs, addon/embedding users will always need to catch up with node changes upon this part.

src/node.cc Outdated
@@ -4425,15 +4425,22 @@ int EmitExit(Environment* env) {
}


IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) {
return new IsolateData(isolate, loop, nullptr);
ArrayBufferAllocatorBase* CreateArrayBufferAllocator() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could also use a std::unique_ptr, and/or drop FreeArrayBufferAllocator because that would be implicit in using std::unique_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed the pattern of the existing APIs like FreeIsolateData, FreeEnvironment and FreePlatform. I think they were made in the current pattern to avoid trouble of unique_ptr for ABI.
Do you think I can keep this pattern for ArrayBufferAllocator?

src/node.h Outdated
NODE_EXTERN IsolateData* CreateIsolateData(
v8::Isolate* isolate,
struct uv_loop_s* loop,
MultiIsolatePlatform* platform);
MultiIsolatePlatform* platform = nullptr,
uint32_t* zero_fill_field = nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fyi, removal of the first variant and adding the field here would be an ABI breakage and therefore semver-major (adding the label for now). I think you might first want to go the route of adding Yet Another Overload, maybe leaving a TODO() comment for switching to default parameters in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the kind reminder. Just added a new overload to avoid semver-major for this PR, and added TODO().

src/node.h Outdated

protected:
uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land.
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intention that we expose zero_fill_field() for public use? I don’t think so. It might be easier/expose less internal API surface to just forward-declare ArrayBufferAllocator and add an ArrayBufferAllocator* parameter to CreateIsolateData()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, no public use of zero_fill_field. Just updated it as your suggestion.

@addaleax
Copy link
Member

Oh, and: Please avoid merge commits. :) It would be great if you could rebase them out of this PR; it you’d prefer not to, the person landing this PR would have to do that, which can get a bit messy if there are merge conflicts along the way.

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels May 11, 2018
src/node.cc Outdated
@@ -4425,10 +4425,21 @@ int EmitExit(Environment* env) {
}


ArrayBufferAllocator* CreateArrayBufferAllocator() {
return new ArrayBufferAllocator;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new ArrayBufferAllocator() - i.e., include parentheses.

src/node.cc Outdated
inline int Start(uv_loop_t* event_loop,
int argc, const char* const* argv,
int exec_argc, const char* const* exec_argv) {
ArrayBufferAllocator* allocator = CreateArrayBufferAllocator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a std::unique_ptr with a custom deleter, or simply use a stack-allocated ArrayBufferAllocator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed the pattern of the existing APIs like FreeIsolateData, FreeEnvironment and FreePlatform. I think they were made in the current pattern to avoid trouble of unique_ptr for ABI.
Do you think I can keep this pattern for ArrayBufferAllocator?

Updated with unique_ptr, and I don't have to customize deleter for usage in node.cc because it can get the full definition here. As to the public API/ABI, do you think I can keep it with raw pointer by following the existing pattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, raw pointers are the way to go, that avoids trouble when Node.js and the add-on use different allocators.

src/node.cc Outdated
@@ -4611,15 +4639,16 @@ inline int Start(uv_loop_t* event_loop,
Locker locker(isolate);
Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);
IsolateData isolate_data(
IsolateData* isolate_data = CreateIsolateData(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a request.

I want it understood that there's no promise we'll carry these APIs forever. If they become a maintenance hassle, we'll simply remove them again in the next major release.

src/node.cc Outdated
inline int Start(uv_loop_t* event_loop,
int argc, const char* const* argv,
int exec_argc, const char* const* exec_argv) {
std::unique_ptr<ArrayBufferAllocator> allocator(CreateArrayBufferAllocator());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer simply ArrayBufferAllocator allocator; if you're not using a custom deleter, no point in heap-allocating it. Likewise for IsolateData.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detail description. I just want to reuse the APIs to help keep consistency between node-main-thread and addons when the relevant logic gets updated in the future.
Updated with custom deleter for both ArrayBufferAllocator and IsolateData.

addaleax pushed a commit that referenced this pull request May 14, 2018
PR-URL: #20639
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
helloshuangzi added a commit to helloshuangzi/node that referenced this pull request May 14, 2018
PR-URL: nodejs#20639
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@helloshuangzi helloshuangzi deleted the nodeAPI branch May 14, 2018 17:47
@addaleax addaleax mentioned this pull request May 14, 2018
addaleax added a commit that referenced this pull request May 14, 2018
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (helloshuangzi) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
addaleax added a commit that referenced this pull request May 15, 2018
Notable Changes:

* **addons**:
  - Fixed a memory leak for users of `AsyncResource` and N-API. (Michael Dawson) [#20668](#20668)
* **assert**:
  - The `error` parameter of `assert.throws()` can be an object containing regular expressions now. (Ruben Bridgewater) [#20485](#20485)
* **crypto**:
  - The `authTagLength` option has been made more flexible. (Tobias Nießen) [#20235](#20235), [#20039](#20039)
* **http**:
  - Handling of `close` and `aborted` events has been made more consistent. (Robert Nagy) [#20075](#20075), [#20611](#20611)
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with Node.js-specific behaviour have been added to the API. (Allen Yonghuang Wang) [#20639](#20639)
  - Node.js `Environment`s clean up resources before exiting now. (Anna Henningsen) [#19377](#19377)
  - Support for multi-threaded embedding has been improved. (Anna Henningsen) [#20542](#20542), [#20539](#20539), [#20541](#20541)
* **esm**:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules. (Gus Caplan) [#20403](#20403)
* **timers**:
  - `timeout.refresh()` has been added to the public API. (Jeremiah Senkpiel) [#20298](#20298)

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 22, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.