Repository navigation
Last exception should be per-isolate #198
Description
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.
Metadata
Metadata
Assignees
Labels
No labels
Activity
addaleax commentedon Mar 24, 2017
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 commentedon Mar 28, 2017
I think if the last exception is per-isolate then so should
napi_get_last_error_info()be - that is, it should accept thenapi_envas its first argument. Thus, we need to store the last error in thenapi_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 commentedon Mar 28, 2017
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 commentedon Mar 28, 2017
@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.
napi: create napi_env as a real structure tied to the isolate
napi: create napi_env as a real structure tied to the isolate
digitalinfinity commentedon Mar 29, 2017
@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 commentedon Mar 29, 2017
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 commentedon Mar 29, 2017
@addaleax ok! Was trying to not make your life harder 😄
napi: create napi_env as a real structure tied to the isolate
napi: create napi_env as a real structure tied to the isolate
napi: create napi_env as a real structure tied to the isolate
napi: create napi_env as a real structure
napi: add napi_env to napi_get_last_error_info()
3 remaining items
napi: create napi_env as a real structure
napi: create napi_env as a real structure (nodejs#198)
napi: create napi_env as a real structure (nodejs#198)
napi: create napi_env as a real structure (nodejs#198)
napi: create napi_env as a real structure
napi: create napi_env as a real structure
napi: create napi_env as a real structure
n-api: create napi_env as a real structure
n-api: create napi_env as a real structure
n-api: create napi_env as a real structure
n-api: create napi_env as a real structure