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.

Sign up for GitHub

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

Jump to bottom

Last exception should be per-isolate #198

Closed
jasongin opened this issue Mar 24, 2017 · 7 comments
Closed

Last exception should be per-isolate #198

jasongin opened this issue Mar 24, 2017 · 7 comments
Assignees
@gabrielschulhof

Comments

@jasongin
Copy link
Member

Currently in the N-API V8 implementation, the last exception is stored as a global. But that won't be correct when Node supports multiple isolates in the process. Then the last exception should be per-isolate.
@jasongin jasongin mentioned this issue Mar 24, 2017
Closed
4 tasks
@addaleax
Copy link
Member

Fwiw, the question of whether Node will eventually support multiple isolates out of the box hasn’t really been settled yet, but I suspect this would be cool to have for embedders like Electron.

@gabrielschulhof
Copy link
Collaborator

gabrielschulhof commented Mar 28, 2017 •
edited
Loading

I think if the last exception is per-isolate then so should napi_get_last_error_info() be - that is, it should accept the napi_env as its first argument. Thus, we need to store the last error in the napi_env. In turn, we should probably tack a structure onto the Isolate using Isolate::SetData, but that does not let you provide a deleter for when the Isolate disappears. I guess it would be up to that which creates new isolates to also notify of their destruction, or up to V8 to provide such a callback in the future.

@mhdawson
Copy link
Member

mhdawson commented Mar 28, 2017 •
edited
Loading

Yes, every napi call should take a napi_env so that we always have that flexibility. Looks like napi_get_last_error_info is the only one where we missed that.

@mhdawson
Copy link
Member

mhdawson commented Mar 28, 2017 •
edited
Loading

@gabrielschulhof is submitting a PR to add napi_env as a paratmeter to nap_get_last_error_info() something you can add to your list ? That is something I think we want in before the PR lands in the core repo.

@gabrielschulhof gabrielschulhof self-assigned this Mar 28, 2017
@gabrielschulhof gabrielschulhof mentioned this issue Mar 28, 2017
Closed
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 28, 2017
napi: create napi_env as a real structure tied to the isolate
0617d8d 
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, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
@gabrielschulhof gabrielschulhof mentioned this issue Mar 28, 2017
Closed
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 28, 2017
napi: create napi_env as a real structure tied to the isolate
7fdbdf6 
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, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
@digitalinfinity
Copy link
Contributor

@gabrielschulhof I think this is a great PR- however, my suggestion is to break it up into 2 PRs. The first one would change the signature of napi_get_last_error_info, but otherwise leave things as is. The second one would contain the rest of the implementation from this PR. My vote is to land the first one in the PR opened against nodejs/master currently, and land the second one after that PR lands, just to make the review of the larger PR smoother. Thoughts?

@addaleax
Copy link
Member

just to make the review of the larger PR smoother

Fwiw so far it looks like I’m the only one actively reviewing the nodejs/master PR, and I don’t mind reviewing #206 as a single chunk (I’ll try to keep on top of the discussions here, too)

@digitalinfinity
Copy link
Contributor

@addaleax ok! Was trying to not make your life harder 😄

gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 29, 2017
napi: create napi_env as a real structure tied to the isolate
8de9c90 
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, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 29, 2017
napi: create napi_env as a real structure tied to the isolate
920ed54 
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, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 29, 2017
napi: create napi_env as a real structure tied to the isolate
5f0c112 
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, but
only if there isn't one already attached to the isolate, perhaps from
the registration of a pervious module.

3. the last exception and the info about the last error are stored in
the napi_env__ struct which is associated with the isolate via
v8::Isolate::SetData().

Fixes nodejs#198
Closes nodejs#206
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 29, 2017
napi: create napi_env as a real structure
cb6a924 
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.

Fixes nodejs#198
Closes nodejs#208
@gabrielschulhof gabrielschulhof mentioned this issue Mar 29, 2017
Merged
jasongin pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 29, 2017
@jasongin
napi: add napi_env to napi_get_last_error_info()
1b56244 
This makes it necessary to pass napi_env to napi_get_last_error_info()
thereby ensuring that errors can be tied to the napi_env in the future.

Re nodejs#198
jasongin pushed a commit that referenced this issue Mar 29, 2017
@jasongin
napi: add napi_env to napi_get_last_error_info() (#211)
e5461a5 
This makes it necessary to pass napi_env to napi_get_last_error_info()
thereby ensuring that errors can be tied to the napi_env in the future.

Re #198
jasongin pushed a commit to jasongin/nodejs that referenced this issue Mar 29, 2017
@jasongin
napi: add napi_env to napi_get_last_error_info() (nodejs#211)
232f004 
This makes it necessary to pass napi_env to napi_get_last_error_info()
thereby ensuring that errors can be tied to the napi_env in the future.

Re nodejs/abi-stable-node#198
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 30, 2017
napi: create napi_env as a real structure
9b27242 
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.

Fixes nodejs#198
Closes nodejs#208
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 30, 2017
napi: create napi_env as a real structure
c22662f 
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.

Fixes nodejs#198
Closes nodejs#208
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Mar 30, 2017
napi: create napi_env as a real structure (nodejs#198)
ce80f95 
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.

Fixes nodejs#198
Closes nodejs#208
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 3, 2017
napi: create napi_env as a real structure (nodejs#198)
e06795b 
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#198
Closes nodejs#208
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 3, 2017
napi: create napi_env as a real structure (nodejs#198)
cf462b5 
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#198
Closes nodejs#208
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 3, 2017
napi: create napi_env as a real structure
8e34f29 
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#198
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 3, 2017
napi: create napi_env as a real structure
36f60eb 
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#198
@gabrielschulhof gabrielschulhof mentioned this issue Apr 3, 2017
Closed
3 tasks
gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 4, 2017
napi: create napi_env as a real structure
6985ed4 
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#198
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 5, 2017
n-api: create napi_env as a real structure
09900a6 
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 pushed a commit to gabrielschulhof/node that referenced this issue Apr 10, 2018
n-api: create napi_env as a real structure
69fe5ae 
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>
MylesBorins pushed a commit to nodejs/node that referenced this issue Apr 16, 2018
@MylesBorins
n-api: create napi_env as a real structure
1e6d3bb 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gabrielschulhof gabrielschulhof

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

napi: create napi_env as a real structure gabrielschulhof/node
5 participants
@digitalinfinity @addaleax @gabrielschulhof @mhdawson @jasongin