src: remove Environment::GetCurrent(isolate)

[anonrig]

Removes Environment::GetCurrent(isolate) calls

cc @nodejs/cpp-reviewers

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/sqlite

[codecov[bot]]

Codecov Report

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

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

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%)	:arrow_down:
src/async_context_frame.cc	100.00% <100.00%> (ø)
src/env-inl.h	94.11% <ø> (-0.08%)	:arrow_down:
src/env.h	98.14% <ø> (ø)
src/node_errors.cc	64.23% <100.00%> (+0.05%)	:arrow_up:
src/node_platform.cc	76.00% <100.00%> (-0.24%)	:arrow_down:
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%)	:arrow_down:
... and 5 more

... and 95 files with indirect coverage changes

:rocket: New features to boost your workflow:

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

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

https://github.com/nodejs/node/blob/20c4b80ffca2738046b21f5792e239ccfa8c9321/src/node.h#L231

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