-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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. | ||||||||
|
||||||||
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. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is an important trait of Lines 228 to 230 in fc054bb
|
||||||||
|
||||||||
### `asyncLocalStorage.getStore()` | ||||||||
|
||||||||
|
@@ -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 | ||||||||
|
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; | ||
}); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 usedisable
.