async_hooks: discourage AsyncLocalStorage.disable

[legendecas]

When --async-context-frame is enabled (and by default in v24), there should be no need to manually call asyncLocalStorage.disable().

Nevertheless, a general AsyncLocalStorage use case will construct an AsyncLocalStorage at module-top-level, so it will never get GC-ed anyway.

If calling asyncLocalStorage.disable() and enable it again, issues like https://github.com/nodejs/node/issues/53037 will rise. So its use should be discouraged.

Refs: https://github.com/nodejs/node/pull/58019

/cc @nodejs/diagnostics

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 90.21%. Comparing base (6cd1c09) to head (25f532c). :warning: Report is 730 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58065      +/-   ##
==========================================
- Coverage   90.21%   90.21%   -0.01%     
==========================================
  Files         630      630              
  Lines      186391   186397       +6     
  Branches    36610    36612       +2     
==========================================
+ Hits       168146   168150       +4     
+ Misses      11066    11053      -13     
- Partials     7179     7194      +15     

Files with missing lines	Coverage Δ
...nternal/async_local_storage/async_context_frame.js	100.00% <100.00%> (ø)

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

[legendecas]

The disable() function is not just for letting the store instance be GC'd, it's also to remove the associated entry from the AsyncContextFrame, allowing the value to be GC'd too. This is still a valid use case.

This statement contradicts with the document:

https://github.com/nodejs/node/blob/fc054bbbb1005c924799c80b07eb0e7ed0278b99/doc/api/async_context.md#L228-L230

The document says asyncLocalStoage.disable does not remove store values from tracked async resources (or with async context frames).

On the tip of main, the behavior also aligns with the doc. It can be verified with a test script:

// Flags: --async-context-frame --expose-gc

'use strict';

const common = require('../common');
const { AsyncLocalStorage } = require('async_hooks');
const { gcUtil, gcUntil } = require('../common/gc');

let weakRef;
{
  const als = new AsyncLocalStorage();
  let obj = {};
  als.run(obj, () => {
    setInterval(() => {}, 1000).unref();
  });
  als.disable();
  weakRef = new WeakRef(obj);
  obj = null;
}

// This assertion fails.
gcUntil('als value', () => {
  return weakRef.deref() == null;
}).then(common.mustCall());

That is, with --async-context-frame, als.disable() does not let its all entered store values being GC-ed. This also applies to flag --no-async-context-frame.

[Flarna]

I'm still not really clear what the expectation of disable() is and if we can reach this target once we have it.

I doubt we find a way to remove all stores from all AsyncContextFrames flying around. In special because an AsyncContextFrame is shared between ALS instances and the creation of them is out of control of each individual ALS instance.

In the old implementation disable() was needed to remove the ALS reference from an internal list. This is needed that it can be GCed but the stores attached to the existing resources stay attached and stay alive - but are no longer propagated to new async resourced.

In the new implementation disable() is a bit like enterWith(undefined). It effects only the current AsyncContextFrame but not other frames and also not the propagation of a store in existing frames. There is no internal array holding a reference to the ALS instance but each and every AsyncContextFrame refer to the store and the ALS instance. Therefore users would have to call disable() at a lot places to clean up and it's not even clear if they can reach all these places.

[Qard]

Depends on the use, but yes, it's not a great API. It's really only implemented to try to keep feature parity with the async_hooks implementation. But the actual usefulness is debatable. More could probably be done to bring the behaviour in line with the async_hooks implementation, but discouraging its use or possibly even deprecating it may also be reasonable. I would say it's not quite the same as enterWith(undefined) though as that will explicitly associate undefined with that store, keeping a strong reference to the store, while disable() will actually remove the strong reference. Could probably drop the strong reference to undefined values though and drop disable() to simplify things. 🤔

[Flarna]

I agree disable() is not exactly the same as enterWith(<defaultStore>/undefined) or exit().

Having an API to just remove the strong reference from the current active AsyncContextFrame but having no control when the same strong reference gets copied into a new AsyncContextFrame (nested run/enterWith on same or other ALS instance) looks like something woring fine in a unit/module test but fails in a real world apps.

I fear a global working disable() API would require to keep track of all AsyncContextFrames via e.g. a wake ref. Haven't tried but that sounds quit expensive.

[Qard]

Given that it's marked experimental currently, and the ergonomics are...not the best...I think it's probably fine to warn against its use, but perhaps provide more clarity about why it continues to exist rather than just being entirely deprecated? Could also just deprecate it anyway, as I'm not sure if anyone is even using it. 🤔