napi: create napi_env as a real structure #12195

gabrielschulhof · 2017-04-03T20:49:30Z

We define struct napi_env__ to include the isolate, the last
exception, and the info about the last error.
We instantiate one struct napi_env__ during module registration and
we pass it into the FunctionCallbackInfo for all subsequent entries into
N-API when we create functions/accessors/finalizers.

Once module unloading will be supported we shall have to delete the
napi_env we create during module init.

There is a clear separation between public and private API wrt. env:

Public APIs assert that env is not nullptr as their first action.
Private APIs need not validate env. They assume it's not nullptr.

Fixes: nodejs/abi-stable-node#198

Checklist

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

Affected core subsystem(s)

napi

gabrielschulhof · 2017-04-03T20:50:13Z

The original PR against abi-stable-node (for reference): nodejs/abi-stable-node#208

gabrielschulhof · 2017-04-03T21:49:15Z

That original PR had already received th LGTM from @addaleax.

mhdawson · 2017-04-03T21:55:08Z

src/node_api.cc

+  v8::Isolate* isolate;
+  v8::Persistent<v8::Value> last_exception;
+  napi_extended_error_info last_error;
+};


I'm thinkintg the base napi_env__ class should be independant of the implementation. It would then be extended by implementations to add specifics like the isolate. In that way the contract would be clear and we later want to stash some vm independent info in the base env we can do that and it would flow automatically into the implementations.

Some implementations may be written in C, so I think the lowest common denominator is the pointer to the struct/class napi_env__, which is defined as napi_env.

I don't think I got my point across quite right. What I meant was that we may want to add elements to the napi env structure which are not related to the implementation (v8, chackra) and have it setup so that the we can do that so that the implementations can just inherit those changes would be good.

Having said that, if that is not straight forward we can defer to a later PR. If you believe thats the case just let me know as the change looks good to me other than that suggestion.

I'm not sure what we could provide to implementations. For example, last_exception only exists in the V8 implementation because in ChakraCore napi_get_and_clear_last_exception() goes straight through to JSRT, so we likely won't need to store any last exception on the env in the ChakraCore implementation.

Of course, we could come up with an API for implementors, wherein we might expose the minimal definition of napi_env__ for them to extend, but I think that's a substantial exercise.

Interestingly though, in my WIP Zephyr.js implementation (updated the link to point to the definition of struct napi_env__) I also store the last exception on the env.

Since it seems like it requires more thought, lets proceed with the PR as is and we can revise if/when we think it through further.

addaleax

LGTM but the comments from nodejs/abi-stable-node#208 (review) still apply (indentation in the addon and maybe a check that the napi_get_value_double fails)

gabrielschulhof · 2017-04-04T04:25:07Z

@addaleax sorry! Had some trouble moving to the upstream repo!

gabrielschulhof · 2017-04-04T05:33:45Z

@addaleax should be good now.

mhdawson

LGTM

mhdawson · 2017-04-04T18:49:45Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/7212/

mhdawson · 2017-04-05T17:02:11Z

Unfortunately this conflicts with the changes I just landed, can you rebase and then I'll land right away

gabrielschulhof · 2017-04-05T19:17:04Z

@mhdawson on it.

1. We define struct napi_env__ to include the isolate, the last exception, and the info about the last error. 2. We instantiate one struct napi_env__ during module registration and we pass it into the FunctionCallbackInfo for all subsequent entries into N-API when we create functions/accessors/finalizers. Once module unloading will be supported we shall have to delete the napi_env we create during module init. There is a clear separation between public and private API wrt. env: 1. Public APIs assert that env is not nullptr as their first action. 2. Private APIs need not validate env. They assume it's not nullptr. Fixes: nodejs/abi-stable-node#198

gabrielschulhof · 2017-04-05T19:23:37Z

@mhdawson done.

mhdawson · 2017-04-05T19:48:19Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7236/

mhdawson · 2017-04-05T21:48:59Z

CI green, landed as 0a5bf4a

1. We define struct napi_env__ to include the isolate, the last exception, and the info about the last error. 2. We instantiate one struct napi_env__ during module registration and we pass it into the FunctionCallbackInfo for all subsequent entries into N-API when we create functions/accessors/finalizers. Once module unloading will be supported we shall have to delete the napi_env we create during module init. There is a clear separation between public and private API wrt. env: 1. Public APIs assert that env is not nullptr as their first action. 2. Private APIs need not validate env. They assume it's not nullptr. PR-URL: #12195 Fixes: nodejs/abi-stable-node#198 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

remove code that is no longer used after n-api: create napi_env as a real structure nodejs#12195

1. We define struct napi_env__ to include the isolate, the last exception, and the info about the last error. 2. We instantiate one struct napi_env__ during module registration and we pass it into the FunctionCallbackInfo for all subsequent entries into N-API when we create functions/accessors/finalizers. Once module unloading will be supported we shall have to delete the napi_env we create during module init. There is a clear separation between public and private API wrt. env: 1. Public APIs assert that env is not nullptr as their first action. 2. Private APIs need not validate env. They assume it's not nullptr. PR-URL: nodejs#12195 Fixes: nodejs/abi-stable-node#198 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

1. We define struct napi_env__ to include the isolate, the last exception, and the info about the last error. 2. We instantiate one struct napi_env__ during module registration and we pass it into the FunctionCallbackInfo for all subsequent entries into N-API when we create functions/accessors/finalizers. Once module unloading will be supported we shall have to delete the napi_env we create during module init. There is a clear separation between public and private API wrt. env: 1. Public APIs assert that env is not nullptr as their first action. 2. Private APIs need not validate env. They assume it's not nullptr. Backport-PR-URL: #19447 PR-URL: #12195 Fixes: nodejs/abi-stable-node#198 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 3, 2017

vsemozhetbyt added addons Issues and PRs related to native addons. node-api Issues and PRs related to the Node-API. labels Apr 3, 2017

mhdawson reviewed Apr 3, 2017

View reviewed changes

addaleax approved these changes Apr 4, 2017

View reviewed changes

gabrielschulhof force-pushed the 198-per-module-environment branch from 8e34f29 to 6985ed4 Compare April 4, 2017 05:33

jasnell approved these changes Apr 4, 2017

View reviewed changes

mhdawson approved these changes Apr 4, 2017

View reviewed changes

gabrielschulhof force-pushed the 198-per-module-environment branch from 6985ed4 to 09900a6 Compare April 5, 2017 19:23

mhdawson self-assigned this Apr 5, 2017

mhdawson closed this Apr 5, 2017

gabrielschulhof deleted the 198-per-module-environment branch April 5, 2017 22:12

italoacasas added the dont-land-on-v7.x label Apr 10, 2017

mhdawson added a commit to mhdawson/io.js that referenced this pull request Apr 13, 2017

n-api: get rid of dead code

4d55bd3

remove code that is no longer used after n-api: create napi_env as a real structure nodejs#12195

mhdawson mentioned this pull request Apr 13, 2017

n-api: get rid of dead code #12398

Closed

2 tasks

addaleax added dont-land-on-v4.x labels Apr 13, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

napi: create napi_env as a real structure #12195

napi: create napi_env as a real structure #12195

gabrielschulhof commented Apr 3, 2017 •

edited

Loading

gabrielschulhof commented Apr 3, 2017

gabrielschulhof commented Apr 3, 2017

mhdawson Apr 3, 2017

gabrielschulhof Apr 4, 2017

mhdawson Apr 4, 2017

mhdawson Apr 4, 2017

gabrielschulhof Apr 4, 2017 •

edited

Loading

gabrielschulhof Apr 4, 2017 •

edited

Loading

mhdawson Apr 4, 2017

addaleax left a comment

gabrielschulhof commented Apr 4, 2017 •

edited

Loading

gabrielschulhof commented Apr 4, 2017

mhdawson left a comment

mhdawson commented Apr 4, 2017

mhdawson commented Apr 5, 2017

gabrielschulhof commented Apr 5, 2017

gabrielschulhof commented Apr 5, 2017

mhdawson commented Apr 5, 2017

mhdawson commented Apr 5, 2017

napi: create napi_env as a real structure #12195

napi: create napi_env as a real structure #12195

Conversation

gabrielschulhof commented Apr 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gabrielschulhof commented Apr 3, 2017

gabrielschulhof commented Apr 3, 2017

mhdawson Apr 3, 2017

Choose a reason for hiding this comment

gabrielschulhof Apr 4, 2017

Choose a reason for hiding this comment

mhdawson Apr 4, 2017

Choose a reason for hiding this comment

mhdawson Apr 4, 2017

Choose a reason for hiding this comment

gabrielschulhof Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

gabrielschulhof Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

mhdawson Apr 4, 2017

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 4, 2017 • edited Loading

gabrielschulhof commented Apr 4, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 4, 2017

mhdawson commented Apr 5, 2017

gabrielschulhof commented Apr 5, 2017

gabrielschulhof commented Apr 5, 2017

mhdawson commented Apr 5, 2017

mhdawson commented Apr 5, 2017

gabrielschulhof commented Apr 3, 2017 •

edited

Loading

gabrielschulhof Apr 4, 2017 •

edited

Loading

gabrielschulhof Apr 4, 2017 •

edited

Loading

gabrielschulhof commented Apr 4, 2017 •

edited

Loading