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 3545 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';constcommon=require('../common');const{ AsyncLocalStorage }=require('async_hooks');const{ gcUtil, gcUntil }=require('../common/gc');letweakRef;{constals=newAsyncLocalStorage();letobj={};als.run(obj,()=>{setInterval(()=>{},1000).unref();});als.disable();weakRef=newWeakRef(obj);obj=null;}// This assertion fails.gcUntil('als value',()=>{returnweakRef.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.

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

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.