src: refactor Environment::GetCurrent() usage

[addaleax]

Make Environment::GetCurrent() return nullptr if the current
Context is not a Node.js context, and for the relevant usage of
this function, either:

• Switch to the better GetCurrent(args) variant
• Turn functions in to no-ops where it makes sense
• Make it a CHECK, i.e. an API requirement, where it make sense
• Leave a TODO comment for verifying what, if anything, is to be done

Checklist

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

[nodejs-github-bot]

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/862/pipeline

[addaleax]

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

[mhdawson]

Just wondering what the plan is on the TODO's. Is it that you'll be looking at each one of them afterwards and submitting additional PRs or that if the CHECK's don't result in any failures/problems after a while they would be removed?

[addaleax]

@mhdawson So … the issue is that each of those requires some thought about how to best address embedder/addon use cases, or how to add wiring so that it won’t be necessary. In particular:

For the node_buffer.cc ones, we could either make it an API requirement to have entered a Node.js context before making these calls (i.e. leave the checks in place), or we could do something that is closer to the author’s intention but may or may not quite be what they expect (e.g. return a Uint8Array with an unmodified prototype).

In the module_wrap cases, I just don’t know the answer (yet?) without reading up more on the possible cases that could occur here, and for the N-API ones it’s also a bit of an issue that there doesn’t quite seem to be any clear definition of what exactly a napi_env conceptually is (I feel like we might want to point to a Node.js Environment rather than an Isolate/Context pair).

[gabrielschulhof]

@addaleax a napi_env is wherein all napi_values live. That is, if I have two napi_envs and I try to manipulate a napi_value coming from one napi_env with the other napi_env It'll crash.

In practice, we store the napi_env on the global object, so it's one napi_env per global object. We retrieve the global object from the Local<Context>, which we receive during module registration. So, basically, the first N-API module that is loaded creates then env against the Local<Context> it receives, and stores it on the global it gets from Context::Global(). Subsequent N-API modules just retrieve this napi_env from the global.

[addaleax]

a napi_env is wherein all napi_values live. That is, if I have two napi_envs and I try to manipulate a napi_value coming from one napi_env with the other napi_env It'll crash.

That would map to a V8 Isolate/JS engine instance. However, N-API also now provides access to some event-loop based mechanisms, which don’t need to relate to those in some specific form, which makes this a bit more complex?

In practice, we store the napi_env on the global object, so it's one napi_env per global object. We retrieve the global object from the Local<Context>, which we receive during module registration. So, basically, the first N-API module that is loaded creates then env against the Local<Context> it receives, and stores it on the global it gets from Context::Global(). Subsequent N-API modules just retrieve this napi_env from the global.

That’s on another level as separation based on JS engine instances, though – there can be multiple Contexts/global objects per instance? I’ll be removing the TODO comment here, though, it doesn’t seem necessary according to this.

[addaleax]

Landed in 4286dcf