src: keep ALS store in AsyncResource::MakeCallback

[nigrosimone]

AsyncResource saves the async context frame when it is created, and MakeCallback() enters it with async_context_frame::Scope. Then node::MakeCallback() opens the callback scope with an undefined frame, so the callback never runs in the saved one. Since AsyncContextFrame is the default (v24), AsyncLocalStorage loses its store in the callbacks of node::AsyncResource. With --no-async-context-frame the store is there.

Now MakeCallback() passes the saved frame to InternalMakeCallback(), as the Node-API AsyncContext already does. The Scope goes away, so every call also saves two Environment lookups and a global handle.

The new test fails without the change: the store is undefined in the callback.

The second commit adds type=AsyncResource and type=Call to the make_callback benchmark added in #66316. Call is a plain v8::Function::Call, what the call costs without Node. benchmark/compare.js, 30 runs, Linux x64:

                                                    confidence improvement accuracy (*)   (**)  (***)
napi/make_callback n=1000000 type='AsyncResource'           ***     36.22 %  ±5.06% ±6.74% ±8.79%
napi/make_callback n=1000000 type='Call'                             0.80 %  ±4.70% ±6.25% ±8.14%
napi/make_callback n=1000000 type='MakeCallback'                     2.77 %  ±6.38% ±8.49% ±11.07%
napi/make_callback n=10000000 type='AsyncResource'          ***     29.34 %  ±4.31% ±5.75% ±7.49%
napi/make_callback n=10000000 type='Call'                            4.00 %  ±5.10% ±6.79% ±8.85%
napi/make_callback n=10000000 type='MakeCallback'                    2.17 %  ±3.37% ±4.49% ±5.85%

Refs: #43038
Refs: nodejs/performance#24

Disclosure: I used Opus 5.5 (Max) as coding assistant

[github-actions]

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

[codecov]

Codecov Report

❌ Patch coverage is 68.18182% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.45%. Comparing base (6866129) to head (78d5c35).
⚠️ Report is 215 commits behind head on main.

Files with missing lines	Patch %	Lines
src/api/async_resource.cc	68.18%	2 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66326      +/-   ##
==========================================
+ Coverage   90.36%   90.45%   +0.09%     
==========================================
  Files         792      791       -1     
  Lines      275498   276571    +1073     
  Branches    52798    53114     +316     
==========================================
+ Hits       248947   250172    +1225     
+ Misses      16976    16799     -177     
- Partials     9575     9600      +25     

Files with missing lines	Coverage Δ
src/api/async_resource.cc	83.01% <68.18%> (-12.54%)	⬇️

... and 115 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.

[nodejs-github-bot]

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

[legendecas]

A test case like this fails with the PR:

'use strict';

const common = require('../../common');
const assert = require('assert');
const { AsyncLocalStorage } = require('async_hooks');
const binding = require(`./build/${common.buildType}/binding`);

const als = new AsyncLocalStorage();
let getterStore;

const object = Object.defineProperty({}, 'methöd', {
  get() {
    getterStore = als.getStore();
    als.enterWith('getter');           // <= this leaks out the MakeCallback
    return () => {};
  },
});

const resource =
  als.run('resource', () => binding.createAsyncResource(object));

let actual;
als.run('caller', () => {
  binding.callViaString(resource);
  actual = [getterStore, als.getStore()];
});

binding.destroyAsyncResource(resource);

assert.deepStrictEqual(actual, ['resource', 'caller']);
// With this PR, this results in ['resource', 'getter'] instead.

[legendecas]

This could call into JS and should be scoped with a async_context_frame::Scope.

[nigrosimone]

Thanks, you are right. Fixed in 78d5c35: the Get() runs in the saved frame again, and your case is in test-async-local-storage.js, it fails without the fix.

[legendecas]

Would you mind confirming if the performance gain still preserve after the change? IIUC it changes the claim in the OP.