-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
lib: add a diagnostic channel for observing AsyncContextFrame.set #58229
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
08e5414
079358b
b571562
ee6f887
f1ca69d
71104b3
1dc7cfd
56f1cc8
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 |
---|---|---|
|
@@ -10,6 +10,9 @@ const { | |
setContinuationPreservedEmbedderData, | ||
} = internalBinding('async_context_frame'); | ||
|
||
const { channel } = require('diagnostics_channel'); | ||
const onSet = channel('async_context_frame.set'); | ||
|
||
let enabled_; | ||
|
||
class ActiveAsyncContextFrame extends SafeMap { | ||
|
@@ -23,6 +26,7 @@ class ActiveAsyncContextFrame extends SafeMap { | |
|
||
static set(frame) { | ||
setContinuationPreservedEmbedderData(frame); | ||
onSet.publish(frame); | ||
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. Should we expose/document the 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. I discussed this with @Qard; he thinks (but can of course speak for himself here 😁) that since all channels are by default experimental, we aren't promising stability in what's returned here. As far as I'm concerned, I would've been fine with not even exposing the parameter at all. 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. One of the probles to never stabilize async_hooks was because it leaks internal. If the frame is not needed it's likely best to remove it. Alternative could be to replace it by a well documented wrapper object exposing only parts of AsyncContextFrame or other data like the cause for the set,... |
||
} | ||
|
||
static exchange(frame) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
'use strict'; | ||
require('../common'); | ||
|
||
if (process.execArgv.includes('--no-async-context-frame')) { | ||
// Skipping test as it is only relevant when async_context_frame is not disabled. | ||
process.exit(0); | ||
} | ||
|
||
const assert = require('assert'); | ||
const { subscribe } = require('diagnostics_channel'); | ||
const { AsyncLocalStorage, AsyncResource } = require('async_hooks'); | ||
|
||
let frameSetCounter = 0; | ||
let lastPublishedFrame = 0; | ||
subscribe('async_context_frame.set', (frame) => { | ||
frameSetCounter++; | ||
lastPublishedFrame = frame; | ||
}); | ||
|
||
const asyncLocalStorage = new AsyncLocalStorage(); | ||
assert.strictEqual(frameSetCounter, 0); | ||
let lastObservedSetCounter = 0; | ||
|
||
function assertFrameWasSet(expectedStore) { | ||
assert(frameSetCounter > lastObservedSetCounter); | ||
assert.strictEqual(lastPublishedFrame?.get(asyncLocalStorage), expectedStore); | ||
lastObservedSetCounter = frameSetCounter; | ||
} | ||
|
||
setImmediate(() => { | ||
// Entering an immediate callback sets the frame. | ||
assertFrameWasSet(undefined); | ||
const store = { foo: 'bar' }; | ||
asyncLocalStorage.enterWith(store); | ||
// enterWith sets the frame. | ||
assertFrameWasSet(store); | ||
assert.strictEqual(asyncLocalStorage.getStore(), store); | ||
|
||
setTimeout(() => { | ||
// Entering a timeout callback sets the frame. | ||
assertFrameWasSet(store); | ||
assert.strictEqual(asyncLocalStorage.getStore(), store); | ||
const res = new AsyncResource('test'); | ||
const store2 = { foo: 'bar2' }; | ||
asyncLocalStorage.enterWith(store2); | ||
// enterWith sets the frame. | ||
assertFrameWasSet(store2); | ||
res.runInAsyncScope(() => { | ||
// runInAsyncScope sets the frame on entry. | ||
assertFrameWasSet(store); | ||
// AsyncResource was constructed before store2 assignment, so it should | ||
// keep reflecting the old store. | ||
assert.strictEqual(asyncLocalStorage.getStore(), store); | ||
}); | ||
// runInAsyncScope sets the frame on exit. | ||
assertFrameWasSet(store2); | ||
}, 10); | ||
}); |
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.
Does this hold true for
await
?As far as I remember
await
or more concrete forPromise
s the activation/setting happens by v8.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.
No, an event won't be published on
await
continuation. If we were to implement that, we'd be right back to usingIsolate::SetPromiseHook
. Fortunately, it is not necessary. V8 appropriately propagates whatever is set as the Continuation Preserved Embedder Data (CPED) into the "then" callbacks. I was a bit vague when saying "native state is kept in sync with change in the active frame" but the idea really is that the derived native state itself should be stored in and retrieved from the AsyncContextFrame (ACF) object, e.g. by defining a private symbol-keyed property on it. This way when V8 changes current ACF as its CPED, the native state automatically tracks and we "only" need to observe when Node sets ACF in CPED for the current execution to ensure the native state in the current ACF object is up to date.Unsurprisingly, I'm working on doing exactly this in the Datadog's profiler if you want to see a C++ example :-)
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.
Are you really interested in the
set()
calls?If I understand this correct it's actually the creation of a new
AsyncContextFrame
- so theALS.enterWith()
andrun()
calls which are of your interest but not theACF.exchange()
calls.exchange()
is called by e.g. timers, nextTick,... - to mimic what v8 does automatically for promises.