lib: make ALS default to AsyncContextFrame

[Qard]

I've marked AsyncContextFrame as stable and made it the default. With this we should see the substantial performance improvements to AsyncLocalStorage as the default case for users on future versions of Node.js. Getting more users on this will also help to identify if there are any perf outliers to tweak further.

I hope to land this in v24. I assume we would generally consider this change as major, but I'm happy to downgrade it to minor if we think it's safe to do so.

(Pushing from the train somewhere between Valencia and Barcelona! 🚅💨)

cc @nodejs/diagnostics APMs have had some time to try this out and there have been no further issues after the initial iteration. I'm taking this to mean it seems to be stable. If you disagree, please speak up.

cc @nodejs/electron-installer Note that this makes use of the ContinuationPreservedEmbedderData API which Chromium also makes use of. This likely means Electron will need to change to default to disabled due to this conflict. I believe it's possible for Electron to do some work on their end to enable layering embedder data providers. If anyone is interested in picking this up to support both please reach out, I'm happy to help however I can! 🙂

[nodejs-github-bot]

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

[mcollina]

lgtm

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (c63255b) to head (a61b8d7).
Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55552      +/-   ##
==========================================
- Coverage   88.43%   88.40%   -0.03%     
==========================================
  Files         654      654              
  Lines      187662   187662              
  Branches    36117    36101      -16     
==========================================
- Hits       165962   165910      -52     
- Misses      14938    14989      +51     
- Partials     6762     6763       +1     

Files with missing lines	Coverage Δ
lib/internal/async_context_frame.js	76.31% <100.00%> (-21.06%)	⬇️
src/node_options.cc	87.44% <100.00%> (ø)

... and 25 files with indirect coverage changes

[nodejs-github-bot]

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

[ShogunPanda]

LGTM!

[Qard]

So we should decide: is this in fact a major change? Or can we consider this minor?

I'm thinking maybe we can land as major in v24, watch for any issues, and if nothing comes up we can decide to call it minor and then backport to v22?

[ShogunPanda]

If we talk about the CLI flags only, the --experimental one removal is probably semver-minor since it's experimental by definition. The introduction of new flag is semver-minor.

Now, except for unforeseen bugs, will this change will break anything in the user code?

[Flarna]

The change should not break anything but it might result in a different graph returned by AsyncLocalStore.
Therefore it holds the risk of breaking user applications, APM tools.

If we would be sure that it returns the same tree always the option --no-async-context-frame would be useless and should be removed.

[ShogunPanda]

I see. Personally I'd love to land this on 22 soon rather than in several months. But if you judge it too risky then I can definitely settle for semver-minor.

[BridgeAR]

LGTM

I suggest we handle it as semver-minor and add the do-not-land-on-x labels to prevent backports. If we later on decide to backport it, that's still a possibility.

[Qard]

If we would be sure that it returns the same tree always the option --no-async-context-frame would be useless and should be removed.

The escape hatch exists specifically for Electron--it has a conflicting use of ContinuationPreservedEmbedderData so they will have to disable the new model until (and if) they can provide a way for both embedded data uses to coexist. Other than that, I don't expect anyone else to need the flag, but it's helpful at least during the transition for people to have an escape hatch if there does turn out to be some logical differences. I'm fairly sure it should be logically equivalent, but in a system that big it's easy to miss some edge cases. 😅

[Flarna]

I assume electron needs/uses a different build not a command line flag.

Therefore if I interpret above statement correct we could move to --experimental-no-async-context-frame. It is easier to remove experimental CLI flags and the flag is just intended as temporary to switch to (still experimental) async hooks.

In general I agree that behavior should not change because of this and I'm fine with semver minor. And I'm also fine with the existing flag. Just throwing in some thoughts.

Besides that we have anyway no tests nor guarantees what AsyncLocalStore does/returns in detail for higher level operations like HTTP requests,... Simple internal changes like moving from nextTick to setImmediate may cause changes in this since always.

[nodejs-github-bot]

Landed in 51ae576