Skip to content

Commit d6234b4

Browse files
joyeecheungtargos
authored andcommitted
inspector: skip promise hook in the inspector async hook
Instead of filtering out promises in the async hooks added for async task tracking, add an internal path to skip adding the promise hook completely for the inspector async hook. The actual user-land promise tracking is already handled by V8 inspector. This prevents the internal promise hook from showing up and creating unnecessary noise when stepping into async execution in the inspector. PR-URL: #57148 Refs: https://issues.chromium.org/issues/390581540 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
1 parent f77069b commit d6234b4

File tree

4 files changed

+11
-19
lines changed

4 files changed

+11
-19
lines changed

lib/async_hooks.js

+5-2
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const AsyncContextFrame = require('internal/async_context_frame');
3434
// Get functions
3535
// For userland AsyncResources, make sure to emit a destroy event when the
3636
// resource gets gced.
37-
const { registerDestroyHook } = internal_async_hooks;
37+
const { registerDestroyHook, kNoPromiseHook } = internal_async_hooks;
3838
const {
3939
asyncWrap,
4040
executionAsyncId,
@@ -90,6 +90,7 @@ class AsyncHook {
9090
this[after_symbol] = after;
9191
this[destroy_symbol] = destroy;
9292
this[promise_resolve_symbol] = promiseResolve;
93+
this[kNoPromiseHook] = false;
9394
}
9495

9596
enable() {
@@ -120,7 +121,9 @@ class AsyncHook {
120121
enableHooks();
121122
}
122123

123-
updatePromiseHookMode();
124+
if (!this[kNoPromiseHook]) {
125+
updatePromiseHookMode();
126+
}
124127

125128
return this;
126129
}

lib/internal/async_hooks.js

+2
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,7 @@ function triggerAsyncId() {
576576
return async_id_fields[kTriggerAsyncId];
577577
}
578578

579+
const kNoPromiseHook = Symbol('kNoPromiseHook');
579580

580581
module.exports = {
581582
executionAsyncId,
@@ -624,4 +625,5 @@ module.exports = {
624625
asyncWrap: {
625626
Providers: async_wrap.Providers,
626627
},
628+
kNoPromiseHook,
627629
};

lib/internal/inspector_async_hook.js

+3-16
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,13 @@
11
'use strict';
22

3-
const {
4-
SafeSet,
5-
} = primordials;
6-
73
let hook;
84
let config;
95

106
function lazyHookCreation() {
117
const inspector = internalBinding('inspector');
128
const { createHook } = require('async_hooks');
139
config = internalBinding('config');
10+
const { kNoPromiseHook } = require('internal/async_hooks');
1411

1512
hook = createHook({
1613
init(asyncId, type, triggerAsyncId, resource) {
@@ -19,32 +16,22 @@ function lazyHookCreation() {
1916
// in https://github.com/nodejs/node/pull/13870#discussion_r124515293,
2017
// this should be fine as long as we call asyncTaskCanceled() too.
2118
const recurring = true;
22-
if (type === 'PROMISE')
23-
this.promiseIds.add(asyncId);
24-
else
25-
inspector.asyncTaskScheduled(type, asyncId, recurring);
19+
inspector.asyncTaskScheduled(type, asyncId, recurring);
2620
},
2721

2822
before(asyncId) {
29-
if (this.promiseIds.has(asyncId))
30-
return;
3123
inspector.asyncTaskStarted(asyncId);
3224
},
3325

3426
after(asyncId) {
35-
if (this.promiseIds.has(asyncId))
36-
return;
3727
inspector.asyncTaskFinished(asyncId);
3828
},
3929

4030
destroy(asyncId) {
41-
if (this.promiseIds.has(asyncId))
42-
return this.promiseIds.delete(asyncId);
4331
inspector.asyncTaskCanceled(asyncId);
4432
},
4533
});
46-
47-
hook.promiseIds = new SafeSet();
34+
hook[kNoPromiseHook] = true;
4835
}
4936

5037
function enable() {

test/parallel/test-inspector-async-call-stack.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ function verifyAsyncHookEnabled(message) {
2727
assert.strictEqual(async_hook_fields[kTotals], 4,
2828
`${async_hook_fields[kTotals]} !== 4: ${message}`);
2929
const promiseHooks = getPromiseHooks();
30-
assert.notDeepStrictEqual(
30+
assert.deepStrictEqual( // Inspector async hooks should not enable promise hooks
3131
promiseHooks, emptyPromiseHooks,
3232
`${message}: promise hooks ${inspect(promiseHooks)}`
3333
);

0 commit comments

Comments
 (0)