Skip to content

inspector: prevent propagation of promise hooks to noPromise hooks #58841

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
28 changes: 18 additions & 10 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ function lookupPublicResource(resource) {
// Used by C++ to call all init() callbacks. Because some state can be setup
// from C++ there's no need to perform all the same operations as in
// emitInitScript.
function emitInitNative(asyncId, type, triggerAsyncId, resource) {
function emitInitNative(asyncId, type, triggerAsyncId, resource, isPromiseHook) {
active_hooks.call_depth += 1;
resource = lookupPublicResource(resource);
// Use a single try/catch for all hooks to avoid setting up one per iteration.
Expand All @@ -199,6 +199,10 @@ function emitInitNative(asyncId, type, triggerAsyncId, resource) {
// eslint-disable-next-line no-var
for (var i = 0; i < active_hooks.array.length; i++) {
if (typeof active_hooks.array[i][init_symbol] === 'function') {
if (isPromiseHook &&
active_hooks.array[i][kNoPromiseHook]) {
continue;
}
active_hooks.array[i][init_symbol](
asyncId, type, triggerAsyncId,
resource,
Expand All @@ -222,7 +226,7 @@ function emitInitNative(asyncId, type, triggerAsyncId, resource) {

// Called from native. The asyncId stack handling is taken care of there
// before this is called.
function emitHook(symbol, asyncId) {
function emitHook(symbol, asyncId, isPromiseHook) {
active_hooks.call_depth += 1;
// Use a single try/catch for all hook to avoid setting up one per
// iteration.
Expand All @@ -232,6 +236,10 @@ function emitHook(symbol, asyncId) {
// eslint-disable-next-line no-var
for (var i = 0; i < active_hooks.array.length; i++) {
if (typeof active_hooks.array[i][symbol] === 'function') {
if (isPromiseHook &&
active_hooks.array[i][kNoPromiseHook]) {
continue;
}
active_hooks.array[i][symbol](asyncId);
}
}
Expand Down Expand Up @@ -321,7 +329,7 @@ function promiseInitHook(promise, parent) {
trackPromise(promise, parent);
const asyncId = promise[async_id_symbol];
const triggerAsyncId = promise[trigger_async_id_symbol];
emitInitScript(asyncId, 'PROMISE', triggerAsyncId, promise);
emitInitScript(asyncId, 'PROMISE', triggerAsyncId, promise, true);
}

function promiseInitHookWithDestroyTracking(promise, parent) {
Expand All @@ -339,14 +347,14 @@ function promiseBeforeHook(promise) {
trackPromise(promise);
const asyncId = promise[async_id_symbol];
const triggerId = promise[trigger_async_id_symbol];
emitBeforeScript(asyncId, triggerId, promise);
emitBeforeScript(asyncId, triggerId, promise, true);
}

function promiseAfterHook(promise) {
trackPromise(promise);
const asyncId = promise[async_id_symbol];
if (hasHooks(kAfter)) {
emitAfterNative(asyncId);
emitAfterNative(asyncId, true);
}
if (asyncId === executionAsyncId()) {
// This condition might not be true if async_hooks was enabled during
Expand All @@ -361,7 +369,7 @@ function promiseAfterHook(promise) {
function promiseResolveHook(promise) {
trackPromise(promise);
const asyncId = promise[async_id_symbol];
emitPromiseResolveNative(asyncId);
emitPromiseResolveNative(asyncId, true);
}

let wantPromiseHook = false;
Expand Down Expand Up @@ -492,7 +500,7 @@ function promiseResolveHooksExist() {
}


function emitInitScript(asyncId, type, triggerAsyncId, resource) {
function emitInitScript(asyncId, type, triggerAsyncId, resource, isPromiseHook = false) {
// Short circuit all checks for the common case. Which is that no hooks have
// been set. Do this to remove performance impact for embedders (and core).
if (!hasHooks(kInit))
Expand All @@ -502,15 +510,15 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) {
triggerAsyncId = getDefaultTriggerAsyncId();
}

emitInitNative(asyncId, type, triggerAsyncId, resource);
emitInitNative(asyncId, type, triggerAsyncId, resource, isPromiseHook);
}


function emitBeforeScript(asyncId, triggerAsyncId, resource) {
function emitBeforeScript(asyncId, triggerAsyncId, resource, isPromiseHook = false) {
pushAsyncContext(asyncId, triggerAsyncId, resource);

if (hasHooks(kBefore))
emitBeforeNative(asyncId);
emitBeforeNative(asyncId, isPromiseHook);
}


Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-inspector-debug-async-hook.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();
const test = require('node:test');
const { NodeInstance } = require('../common/inspector-helper');

const script = `
import { createHook } from "async_hooks"
import fs from "fs"
const hook = createHook({
after() {
}
});
hook.enable(true);
console.log('Async hook enabled');
`;

test('inspector async hooks should not crash in debug build', async () => {
const instance = new NodeInstance([
'--inspect-brk=0',
], script);
const session = await instance.connectInspectorSession();
await session.send({ method: 'NodeRuntime.enable' });
await session.waitForNotification('NodeRuntime.waitingForDebugger');
await session.send({ method: 'Runtime.enable' });
await session.send({ method: 'Debugger.enable' });
await session.send({ id: 6, method: 'Debugger.setAsyncCallStackDepth', params: { maxDepth: 32 } });
await session.send({ method: 'Runtime.runIfWaitingForDebugger' });
await session.waitForDisconnect();
});
Loading