From 7aad63ba349d7b9b0eda4149d236ee66e5a26401 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 26 Mar 2019 16:03:27 -0400 Subject: [PATCH] src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt If Debugger.setAsyncCallStackDepth is sent during bootstrap, we cannot immediately call into JS to enable the hooks, which could interrupt the JS execution of bootstrap. So instead we save the notification in the inspector agent if it's sent in the middle of bootstrap, and process the notification later here. Refs: https://github.com/nodejs/node/issues/26798 PR-URL: https://github.com/nodejs/node/pull/26935 Reviewed-By: Anna Henningsen Reviewed-By: Eugene Ostroukhov Reviewed-By: Minwoo Jung --- lib/internal/bootstrap/node.js | 11 ----------- lib/internal/bootstrap/pre_execution.js | 18 +++++++++++++++++- lib/internal/main/worker_thread.js | 2 ++ src/inspector_agent.cc | 1 + 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 5cc24980faf..abd88afce05 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -157,17 +157,6 @@ if (isMainThread) { const { nativeHooks } = require('internal/async_hooks'); internalBinding('async_wrap').setupHooks(nativeHooks); -// XXX(joyeecheung): this has to be done after the initial load of -// `internal/async_hooks` otherwise `async_hooks` cannot require -// `internal/async_hooks`. Investigate why. -if (config.hasInspector) { - const { - enable, - disable - } = require('internal/inspector_async_hook'); - internalBinding('inspector').registerAsyncHook(enable, disable); -} - const { setupTaskQueue, queueMicrotask diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 20b362f2136..ab7e169d7ac 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -7,7 +7,7 @@ function prepareMainThreadExecution() { // Patch the process object with legacy properties and normalizations patchProcessObject(); setupTraceCategoryState(); - + setupInspectorHooks(); setupWarningHandler(); // Resolve the coverage directory to an absolute path, and @@ -168,6 +168,21 @@ function setupTraceCategoryState() { toggleTraceCategoryState(isTraceCategoryEnabled('node.async_hooks')); } +function setupInspectorHooks() { + // If Debugger.setAsyncCallStackDepth is sent during bootstrap, + // we cannot immediately call into JS to enable the hooks, which could + // interrupt the JS execution of bootstrap. So instead we save the + // notification in the inspector agent if it's sent in the middle of + // bootstrap, and process the notification later here. + if (internalBinding('config').hasInspector) { + const { + enable, + disable + } = require('internal/inspector_async_hook'); + internalBinding('inspector').registerAsyncHook(enable, disable); + } +} + // In general deprecations are intialized wherever the APIs are implemented, // this is used to deprecate APIs implemented in C++ where the deprecation // utitlities are not easily accessible. @@ -361,5 +376,6 @@ module.exports = { initializeFrozenIntrinsics, loadPreloadModules, setupTraceCategoryState, + setupInspectorHooks, initializeReport }; diff --git a/lib/internal/main/worker_thread.js b/lib/internal/main/worker_thread.js index fd3502e0784..71babafd0e8 100644 --- a/lib/internal/main/worker_thread.js +++ b/lib/internal/main/worker_thread.js @@ -6,6 +6,7 @@ const { patchProcessObject, setupCoverageHooks, + setupInspectorHooks, setupWarningHandler, setupDebugEnv, initializeDeprecations, @@ -43,6 +44,7 @@ const { const publicWorker = require('worker_threads'); patchProcessObject(); +setupInspectorHooks(); setupDebugEnv(); const debug = require('internal/util/debuglog').debuglog('worker'); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 90ed2857815..91a57b9944e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -835,6 +835,7 @@ void Agent::DisableAsyncHook() { void Agent::ToggleAsyncHook(Isolate* isolate, const node::Persistent& fn) { + CHECK(parent_env_->has_run_bootstrapping_code()); HandleScope handle_scope(isolate); CHECK(!fn.IsEmpty()); auto context = parent_env_->context();