Last exception should be per-isolate

[jasongin]

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.

[addaleax]

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]

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]

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]

@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.

[digitalinfinity]

@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]

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]

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