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
#53037 will rise. So its use should be discouraged.

Refs: #58019

/cc @nodejs/diagnostics

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (6cd1c09) to head (25f532c).
Report is 2 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

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

[Qard]

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.

With this change, the symbol is held alive instead, and the value remains in the map.

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

node/doc/api/async_context.md

Lines 228 to 230 in fc054bb

	`asyncLocalStorage` can be garbage collected. This does not apply to stores
	provided by the `asyncLocalStorage`, as those objects are garbage collected
	along with the corresponding async resources.

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.

[jasnell]

The wording here is a bit off. If the intention is to say that disable() is not needed to allow the ALS instance to be garbage collected then I would actually suggest just dropping this first sentence.

[legendecas]

This only works with --async-context-frame. Given that --async-context-frame is the default option now, it is simply discouraged to use disable.

[jasnell]

This second sentence here is rather confusing if you're not familiar with ALS as a whole. I'm not sure it adds a lot of value here.

[legendecas]

It is an important trait of disable that it does not make all entered stores GC-able. It is an existing document sentence that re-organized in this PR:

node/doc/api/async_context.md

Lines 228 to 230 in fc054bb

	`asyncLocalStorage` can be garbage collected. This does not apply to stores
	provided by the `asyncLocalStorage`, as those objects are garbage collected
	along with the corresponding async resources.