inspector: track async stacks when necessary

With this change, we do async stack tracking only when explicitly requested by the inspector client. This avoids unnecessary overhead for clients that might not be interested in async stack traces. Backport-PR-URL: #16590 PR-URL: #16308Fixes: #16180 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]>

Author: ofrobots

lib/internal/inspector_async_hook.js

@@ -55,10 +55,4 @@ function disable(){
 
 exports.setup=function(){
 inspector.registerAsyncHook(enable,disable);
-
-if(inspector.isEnabled()){
-// If the inspector was already enabled via --inspect or --inspect-brk,
-// the we need to enable the async hook immediately at startup.
-enable();
-}
 };

src/inspector_agent.cc

@@ -319,6 +319,14 @@ class NodeInspectorClient : public V8InspectorClient{
 returnuv_hrtime() * 1.0 / NANOS_PER_MSEC;
  }
 
+voidmaxAsyncCallStackDepthChanged(int depth) override{
+if (depth == 0){
+ env_->inspector_agent()->DisableAsyncHook();
+ } else{
+ env_->inspector_agent()->EnableAsyncHook();
+ }
+ }
+
 voidcontextCreated(Local<Context> context, const std::string& name){
  std::unique_ptr<StringBuffer> name_buffer = Utf8ToStringView(name);
  v8_inspector::V8ContextInfo info(context, CONTEXT_GROUP_ID,
@@ -449,7 +457,9 @@ Agent::Agent(Environment* env) : parent_env_(env),
  client_(nullptr),
  platform_(nullptr),
  enabled_(false),
- next_context_number_(1){}
+ next_context_number_(1),
+ pending_enable_async_hook_(false),
+ pending_disable_async_hook_(false){}
 
 // Destructor needs to be defined here in implementation file as the header
 // does not have full definition of some classes.
@@ -498,17 +508,6 @@ bool Agent::StartIoThread(bool wait_for_connect){
  HandleScope handle_scope(isolate);
 auto context = parent_env_->context();
 
-// Enable tracking of async stack traces
-if (!enable_async_hook_function_.IsEmpty()){
- Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
-auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
-if (result.IsEmpty()){
-FatalError(
-"node::InspectorAgent::StartIoThread",
-"Cannot enable Inspector's AsyncHook, please report this.");
- }
- }
-
 // Send message to enable debug in workers
  Local<Object> process_object = parent_env_->process_object();
  Local<Value> emit_fn =
@@ -537,38 +536,9 @@ void Agent::Stop(){
  io_.reset();
  enabled_ = false;
  }
-
- v8::Isolate* isolate = parent_env_->isolate();
- HandleScope handle_scope(isolate);
-
-// Disable tracking of async stack traces
-if (!disable_async_hook_function_.IsEmpty()){
- Local<Function> disable_fn = disable_async_hook_function_.Get(isolate);
-auto result = disable_fn->Call(parent_env_->context(),
-Undefined(parent_env_->isolate()), 0, nullptr);
-if (result.IsEmpty()){
-FatalError(
-"node::InspectorAgent::Stop",
-"Cannot disable Inspector's AsyncHook, please report this.");
- }
- }
 }
 
 voidAgent::Connect(InspectorSessionDelegate* delegate){
-if (!enabled_){
-// Enable tracking of async stack traces
- v8::Isolate* isolate = parent_env_->isolate();
- HandleScope handle_scope(isolate);
-auto context = parent_env_->context();
- Local<Function> enable_fn = enable_async_hook_function_.Get(isolate);
-auto result = enable_fn->Call(context, Undefined(isolate), 0, nullptr);
-if (result.IsEmpty()){
-FatalError(
-"node::InspectorAgent::Connect",
-"Cannot enable Inspector's AsyncHook, please report this.");
- }
- }
-
  enabled_ = true;
  client_->connectFrontend(delegate);
 }
@@ -626,6 +596,50 @@ void Agent::RegisterAsyncHook(Isolate* isolate,
  v8::Local<v8::Function> disable_function){
  enable_async_hook_function_.Reset(isolate, enable_function);
  disable_async_hook_function_.Reset(isolate, disable_function);
+if (pending_enable_async_hook_){
+CHECK(!pending_disable_async_hook_);
+ pending_enable_async_hook_ = false;
+EnableAsyncHook();
+ } elseif (pending_disable_async_hook_){
+CHECK(!pending_enable_async_hook_);
+ pending_disable_async_hook_ = false;
+DisableAsyncHook();
+ }
+}
+
+voidAgent::EnableAsyncHook(){
+if (!enable_async_hook_function_.IsEmpty()){
+ Isolate* isolate = parent_env_->isolate();
+ToggleAsyncHook(isolate, enable_async_hook_function_.Get(isolate));
+ } elseif (pending_disable_async_hook_){
+CHECK(!pending_enable_async_hook_);
+ pending_disable_async_hook_ = false;
+ } else{
+ pending_enable_async_hook_ = true;
+ }
+}
+
+voidAgent::DisableAsyncHook(){
+if (!disable_async_hook_function_.IsEmpty()){
+ Isolate* isolate = parent_env_->isolate();
+ToggleAsyncHook(isolate, disable_async_hook_function_.Get(isolate));
+ } elseif (pending_enable_async_hook_){
+CHECK(!pending_disable_async_hook_);
+ pending_enable_async_hook_ = false;
+ } else{
+ pending_disable_async_hook_ = true;
+ }
+}
+
+voidAgent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn){
+ HandleScope handle_scope(isolate);
+auto context = parent_env_->context();
+auto result = fn->Call(context, Undefined(isolate), 0, nullptr);
+if (result.IsEmpty()){
+FatalError(
+"node::inspector::Agent::ToggleAsyncHook",
+"Cannot toggle Inspector's AsyncHook, please report this.");
+ }
 }
 
 voidAgent::AsyncTaskScheduled(const StringView& task_name, void* task,

src/inspector_agent.h

@@ -92,7 +92,12 @@ class Agent{
  DebugOptions& options(){return debug_options_}
 voidContextCreated(v8::Local<v8::Context> context);
 
+voidEnableAsyncHook();
+voidDisableAsyncHook();
+
 private:
+voidToggleAsyncHook(v8::Isolate* isolate, v8::Local<v8::Function> fn);
+
  node::Environment* parent_env_;
  std::unique_ptr<NodeInspectorClient> client_;
  std::unique_ptr<InspectorIo> io_;
@@ -102,6 +107,8 @@ class Agent{
  DebugOptions debug_options_;
 int next_context_number_;
 
+bool pending_enable_async_hook_;
+bool pending_disable_async_hook_;
  v8::Persistent<v8::Function> enable_async_hook_function_;
  v8::Persistent<v8::Function> disable_async_hook_function_;
 };

test/sequential/sequential.status

@@ -7,6 +7,7 @@ prefix sequential
 [true] # This section applies to all platforms
 
 [$system==win32]
+test-inspector-async-call-stack : PASS, FLAKY
 test-inspector-bindings : PASS, FLAKY
 test-inspector-debug-end : PASS, FLAKY
 test-inspector-stop-profile-after-done: PASS, FLAKY

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

@@ -0,0 +1,79 @@
+'use strict';
+constcommon=require('../common');
+common.skipIfInspectorDisabled();
+common.skipIf32Bits();
+
+constassert=require('assert');
+constasync_wrap=process.binding('async_wrap');
+const{ kTotals }=async_wrap.constants;
+constinspector=require('inspector');
+
+constsetDepth='Debugger.setAsyncCallStackDepth';
+
+functionverifyAsyncHookDisabled(message){
+assert.strictEqual(async_wrap.async_hook_fields[kTotals],0);
+}
+
+functionverifyAsyncHookEnabled(message){
+assert.strictEqual(async_wrap.async_hook_fields[kTotals],4);
+}
+
+// By default inspector async hooks should not have been installed.
+verifyAsyncHookDisabled('inspector async hook should be disabled at startup');
+
+constsession=newinspector.Session();
+verifyAsyncHookDisabled('creating a session should not enable async hooks');
+
+session.connect();
+verifyAsyncHookDisabled('connecting a session should not enable async hooks');
+
+session.post('Debugger.enable',()=>{
+verifyAsyncHookDisabled('enabling debugger should not enable async hooks');
+
+session.post(setDepth,{invalid: 'message'},()=>{
+verifyAsyncHookDisabled('invalid message should not enable async hooks');
+
+session.post(setDepth,{maxDepth: 'five'},()=>{
+verifyAsyncHookDisabled('invalid maxDepth (string) should not enable '+
+'async hooks');
+
+session.post(setDepth,{maxDepth: NaN},()=>{
+verifyAsyncHookDisabled('invalid maxDepth (NaN) should not enable '+
+'async hooks');
+
+session.post(setDepth,{maxDepth: 10},()=>{
+verifyAsyncHookEnabled('valid message should enable async hooks');
+
+session.post(setDepth,{maxDepth: 0},()=>{
+verifyAsyncHookDisabled('Setting maxDepth to 0 should disable '+
+'async hooks');
+
+runTestSet2(session);
+});
+});
+});
+});
+});
+});
+
+functionrunTestSet2(session){
+session.post(setDepth,{maxDepth: 32},()=>{
+verifyAsyncHookEnabled('valid message should enable async hooks');
+
+session.post('Debugger.disable',()=>{
+verifyAsyncHookDisabled('Debugger.disable should disable async hooks');
+
+session.post('Debugger.enable',()=>{
+verifyAsyncHookDisabled('Enabling debugger should not enable hooks');
+
+session.post(setDepth,{maxDepth: 64},()=>{
+verifyAsyncHookEnabled('valid message should enable async hooks');
+
+session.disconnect();
+verifyAsyncHookDisabled('Disconnecting session should disable '+
+'async hooks');
+});
+});
+});
+});
+}