Skip to content

async_hooks: discourage AsyncLocalStorage.disable #58065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions doc/api/async_context.md
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,14 @@ to `asyncLocalStorage.getStore()` will return `undefined` until
When calling `asyncLocalStorage.disable()`, all current contexts linked to the
instance will be exited.

Calling `asyncLocalStorage.disable()` is required before the
`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.
There is no need to call this method in order to get an `asyncLocalStorage`
instance from being garbage-collected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Use this method when the `asyncLocalStorage` is not in use anymore
in the current process.
When running Node.js with CLI flag [`--no-async-context-frame`][], calling
`asyncLocalStorage.disable()` is required before the `asyncLocalStorage` itself
can be garbage collected. However, this does not apply to stores provided by
the `asyncLocalStorage`, as those objects are garbage collected along with the
corresponding async resources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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


### `asyncLocalStorage.getStore()`

Expand Down Expand Up @@ -905,6 +906,7 @@ const server = createServer((req, res) => {
}).listen(3000);
```

[`--no-async-context-frame`]: cli.md#--no-async-context-frame
[`AsyncResource`]: #class-asyncresource
[`EventEmitter`]: events.md#class-eventemitter
[`Stream`]: stream.md#stream
Expand Down
14 changes: 10 additions & 4 deletions lib/internal/async_local_storage/async_context_frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const {
ReflectApply,
Symbol,
} = primordials;

const {
Expand All @@ -12,6 +13,7 @@ const AsyncContextFrame = require('internal/async_context_frame');
const { AsyncResource } = require('async_hooks');

class AsyncLocalStorage {
#frameKey;
#defaultValue = undefined;
#name = undefined;

Expand All @@ -30,6 +32,8 @@ class AsyncLocalStorage {
if (options.name !== undefined) {
this.#name = `${options.name}`;
}

this.#frameKey = Symbol(this.#name ? `AsyncLocalStorage ${this.#name}` : 'AsyncLocalStorage');
}

/** @type {string} */
Expand All @@ -44,11 +48,13 @@ class AsyncLocalStorage {
}

disable() {
AsyncContextFrame.disable(this);
// TODO(legendecas): deprecate this method once `--async-context-frame` is
// the only way to use AsyncLocalStorage.
AsyncContextFrame.disable(this.#frameKey);
}

enterWith(data) {
const frame = new AsyncContextFrame(this, data);
const frame = new AsyncContextFrame(this.#frameKey, data);
AsyncContextFrame.set(frame);
}

Expand All @@ -68,10 +74,10 @@ class AsyncLocalStorage {

getStore() {
const frame = AsyncContextFrame.current();
if (!frame?.has(this)) {
if (!frame?.has(this.#frameKey)) {
return this.#defaultValue;
}
return frame?.get(this);
return frame?.get(this.#frameKey);
}
}

Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-async-local-storage-gc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Flags: --async-context-frame --expose-gc --expose-internals

'use strict';
const common = require('../common');
const { gcUntil } = require('../common/gc');
const { AsyncLocalStorage } = require('async_hooks');
const AsyncContextFrame = require('internal/async_context_frame');

// To be compatible with `test-without-async-context-frame.mjs`.
if (!AsyncContextFrame.enabled) {
common.skip('AsyncContextFrame is not enabled');
}

// Verify that the AsyncLocalStorage object can be garbage collected even without
// `asyncLocalStorage.disable()` being called, when `--async-context-frame` is enabled.

let weakRef = null;
{
const alsValue = {};
let als = new AsyncLocalStorage();
als.run(alsValue, () => {
setInterval(() => {
/**
* Empty interval to keep the als value alive.
*/
}, 1000).unref();
});
weakRef = new WeakRef(als);
als = null;
}

gcUntil('AsyncLocalStorage object can be garbage collected', () => {
return weakRef.deref() === undefined;
});
Loading