src: remove Environment::GetCurrent(isolate)

[anonrig]

Removes Environment::GetCurrent(isolate) calls

cc @nodejs/cpp-reviewers

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 88.09524% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (2281a04) to head (ea46cf2).
⚠️ Report is 1479 commits behind head on main.

Files with missing lines	Patch %	Lines
src/api/hooks.cc	80.00%	2 Missing ⚠️
src/api/callback.cc	75.00%	0 Missing and 1 partial ⚠️
src/api/environment.cc	50.00%	0 Missing and 1 partial ⚠️
src/node_buffer.cc	80.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #58311 +/- ## ========================================== - Coverage 90.21% 89.68% -0.54%  ========================================== Files 633 633 Lines 186834 186832 -2 Branches 36683 36363 -320 ========================================== - Hits 168549 167556 -993 - Misses 11089 12023 +934 - Partials 7196 7253 +57 

Files with missing lines	Coverage Δ
src/api/async_resource.cc	95.23% <100.00%> (ø)
src/api/exceptions.cc	90.69% <100.00%> (-3.49%)	⬇️
src/async_context_frame.cc	100.00% <100.00%> (ø)
src/env-inl.h	94.11% <ø> (-0.08%)	⬇️
src/env.h	98.14% <ø> (ø)
src/node_errors.cc	64.23% <100.00%> (+0.05%)	⬆️
src/node_platform.cc	76.00% <100.00%> (-0.24%)	⬇️
src/node_report.cc	93.23% <100.00%> (ø)
src/node_sqlite.cc	80.58% <100.00%> (ø)
src/node_task_queue.cc	83.48% <100.00%> (-0.92%)	⬇️
... and 5 more

... and 95 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasnell]

Defensively marking this semver-major as this may be an ABI-breaking changing. Environment is used via node.h and directly depended on by native addons that might be using this method. Instead of removing right away we likely need to take it through a proper deprecation cycle.

Putting the red x on this until the possible downstream impact can be determined.

[targos]

@jasnell I don't think what you wrote is correct. env.h only declares Environment to use it as a pointer. We don't expose env.h in the public headers.

node/src/node.h

Line 231 in 20c4b80

classEnvironment;

[jasnell]

While that is true, native addons do make use of Environment directly. A quick search on cs.github.com reveals a number of native addons that directly use this API.

[targos]

Can you post a couple examples ?

[jasnell]

https://github.com/search?type=code&q=%22node%3A%3AEnvironment%3A%3AGetCurrent%28%22+language%3AC%2B%2B&l=C%2B%2B

[anonrig]

https://github.com/search?type=code&q=%22node%3A%3AEnvironment%3A%3AGetCurrent%28%22+language%3AC%2B%2B&l=C%2B%2B

I see that most usage comes from Electron. Although, this might look like a breaking change, this function isn't exposed using similar. I wonder if semver-major holds for functions that are not publicly exposed.

The reason I want to remove this because this is an anti-pattern. Removing isolate constructor, and accessing through isolate's context gives the same outcome, but gives the option to create a handleScope or not, to the user.

My 2 cents, it to move forward with this, and later put environment into slow/fast path callback data, and eventually getting rid of similar usages (and potentially limiting the creation scope of environment object)

@nodejs/tsc any input is appreciated

[targos]

It's not clear to me that there are legitimate native addons in the search result. I only see Node.js source code and Electron source code.

[jasnell]

Electron's use doesn't count? At this point I'm not comfortable lifting my -1 on this. I think it should go through at least a deprecation cycle (deprecate now, removed only after the next major release)

[joyeecheung]

If the idea is to avoid creating the HandleScope don't think the changes here do the job

and accessing through isolate's context gives the same outcome, but gives the option to create a handleScope or not, to the user.

The HandleScope is needed because the v8::Isolate::GetCurrentContext() call needs to return the context in a Local<Context> handle (as you can see from the failures), and this PR is merely inlining that call, not removing the need to create a HandleScope. The right way to avoid the HandleScope is, of course, avoid creating any handles, which includes avoiding calling v8::Isolate::GetCurrentContext() at all - or not depending on any context since contexts are passed around in handles for that matter. Environment::GetCurrent(isolate) is just one of the many things that call v8::Isolate::GetCurrentContext(). The problem for fast API is not Environment, it's the context, and you are only seeing it from Environment::GetCurrent since the Envrionment is looked up from the context.

The reason I want to remove this because this is an anti-pattern.

I think the anti-pattern is to require an Environment when all the binding needs is only the isolate, or the context. If the binding really needs an Environment/the context (many of the code being touched here do) then a HandleScope must still be created to hold Local<Context>. But if it just needs the isolate or if it doesn't even need the isolate then it doesn't need an Environment or a context or a HandleScope. The bindings being touched here still rely on the Environment and the context so it doesn't really improve much other than making the code more verbose and making it crash since it's not safe to create Local<Context> without a handle scope.

[joyeecheung]

For example, this is not doing what the comment above claims. The code here still needs the Environment, therefore it still needs to look it up from the context, therefore it still needs a HandleScope. It's not up to the caller to choose to create it or not - if it doesn't, V8 can just crash.