Skip to content

Commit

Permalink
src: do not call into JS in the maxAsyncCallStackDepthChanged interrupt
Browse files Browse the repository at this point in the history
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: nodejs#26798
  • Loading branch information
joyeecheung committed Mar 26, 2019
1 parent 332032c commit d5f9395
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 12 deletions.
11 changes: 0 additions & 11 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 17 additions & 1 deletion lib/internal/bootstrap/pre_execution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -357,5 +372,6 @@ module.exports = {
initializeFrozenIntrinsics,
loadPreloadModules,
setupTraceCategoryState,
setupInspectorHooks,
initializeReport
};
2 changes: 2 additions & 0 deletions lib/internal/main/worker_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
const {
patchProcessObject,
setupCoverageHooks,
setupInspectorHooks,
setupWarningHandler,
setupDebugEnv,
initializeDeprecations,
Expand Down Expand Up @@ -43,6 +44,7 @@ const {
const publicWorker = require('worker_threads');

patchProcessObject();
setupInspectorHooks();
setupDebugEnv();

const debug = require('internal/util/debuglog').debuglog('worker');
Expand Down
1 change: 1 addition & 0 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,7 @@ void Agent::DisableAsyncHook() {

void Agent::ToggleAsyncHook(Isolate* isolate,
const node::Persistent<Function>& fn) {
CHECK(parent_env_->has_run_bootstrapping_code());
HandleScope handle_scope(isolate);
CHECK(!fn.IsEmpty());
auto context = parent_env_->context();
Expand Down

0 comments on commit d5f9395

Please sign in to comment.