From 700459e008e2cf813bf13060721042f635616160 Mon Sep 17 00:00:00 2001 From: Aleksei Koziatinskii Date: Mon, 6 May 2019 19:30:44 -0700 Subject: [PATCH] inspector: added NodeRuntime domain Historically Node process sends Runtime.executionContextDestroyed with main context as argument when it is finished. This approach has some disadvantages. V8 prevents running some protocol command on destroyed contexts, e.g. Runtime.evaluate will return an error or Debugger.enable won't return a list of scripts. Both command might be useful for different tools, e.g. tool runs Profiler.startPreciseCoverage and at the end of node process it would like to get list of all scripts to match data to source code. Or some tooling frontend would like to provide capabilities to run commands in console when node process is finished to allow user to inspect state of the program at exit. This PR adds new domain: NodeRuntime. After NodeRuntime.notifyWhenWaitingForDisconnect is enabled by at least one client, node will send NodeRuntime.waitingForDebuggerToDisconnect event instead of Runtime.executionContextDestroyed. Based on this signal any protocol client can capture all required information and then disconnect its session. PR-URL: https://github.com/nodejs/node/pull/27600 Reviewed-By: Eugene Ostroukhov Reviewed-By: Rich Trott --- src/inspector/node_inspector.gypi | 4 ++ src/inspector/node_protocol.pdl | 13 ++++++ src/inspector/runtime_agent.cc | 36 +++++++++++++++ src/inspector/runtime_agent.h | 32 +++++++++++++ src/inspector_agent.cc | 46 +++++++++++++++++-- test/common/inspector-helper.js | 4 ++ .../test-inspector-waiting-for-disconnect.js | 45 ++++++++++++++++++ 7 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 src/inspector/runtime_agent.cc create mode 100644 src/inspector/runtime_agent.h create mode 100644 test/parallel/test-inspector-waiting-for-disconnect.js diff --git a/src/inspector/node_inspector.gypi b/src/inspector/node_inspector.gypi index 6ec85461dda0d7..2ee8cfd7dafe3f 100644 --- a/src/inspector/node_inspector.gypi +++ b/src/inspector/node_inspector.gypi @@ -9,6 +9,8 @@ '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeWorker.h', '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.cpp', '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeTracing.h', + '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeRuntime.cpp', + '<(SHARED_INTERMEDIATE_DIR)/src/node/inspector/protocol/NodeRuntime.h', ], 'node_protocol_files': [ '<(protocol_tool_path)/lib/Allocator_h.template', @@ -55,6 +57,8 @@ '../../src/inspector/main_thread_interface.h', '../../src/inspector/node_string.cc', '../../src/inspector/node_string.h', + '../../src/inspector/runtime_agent.cc', + '../../src/inspector/runtime_agent.h', '../../src/inspector/tracing_agent.cc', '../../src/inspector/tracing_agent.h', '../../src/inspector/worker_agent.cc', diff --git a/src/inspector/node_protocol.pdl b/src/inspector/node_protocol.pdl index 9fb9f1c55fa191..36d528b937a038 100644 --- a/src/inspector/node_protocol.pdl +++ b/src/inspector/node_protocol.pdl @@ -92,3 +92,16 @@ experimental domain NodeWorker # Identifier of a session which sends a message. SessionID sessionId string message + +# Support for inspecting node process state. +experimental domain NodeRuntime + # Enable the `NodeRuntime.waitingForDisconnect`. + command notifyWhenWaitingForDisconnect + parameters + boolean enabled + + # This event is fired instead of `Runtime.executionContextDestroyed` when + # enabled. + # It is fired when the Node process finished all code execution and is + # waiting for all frontends to disconnect. + event waitingForDisconnect diff --git a/src/inspector/runtime_agent.cc b/src/inspector/runtime_agent.cc new file mode 100644 index 00000000000000..f8fdbe42d41e03 --- /dev/null +++ b/src/inspector/runtime_agent.cc @@ -0,0 +1,36 @@ +#include "runtime_agent.h" + +#include "env-inl.h" +#include "inspector_agent.h" + +namespace node { +namespace inspector { +namespace protocol { + +RuntimeAgent::RuntimeAgent(Environment* env) + : notify_when_waiting_for_disconnect_(false), env_(env) {} + +void RuntimeAgent::Wire(UberDispatcher* dispatcher) { + frontend_ = std::make_unique(dispatcher->channel()); + NodeRuntime::Dispatcher::wire(dispatcher, this); +} + +DispatchResponse RuntimeAgent::notifyWhenWaitingForDisconnect(bool enabled) { + if (!env_->owns_process_state()) { + return DispatchResponse::Error( + "NodeRuntime domain can only be used through main thread sessions"); + } + notify_when_waiting_for_disconnect_ = enabled; + return DispatchResponse::OK(); +} + +bool RuntimeAgent::notifyWaitingForDisconnect() { + if (notify_when_waiting_for_disconnect_) { + frontend_->waitingForDisconnect(); + return true; + } + return false; +} +} // namespace protocol +} // namespace inspector +} // namespace node diff --git a/src/inspector/runtime_agent.h b/src/inspector/runtime_agent.h new file mode 100644 index 00000000000000..63fc2a897caf21 --- /dev/null +++ b/src/inspector/runtime_agent.h @@ -0,0 +1,32 @@ +#ifndef SRC_INSPECTOR_RUNTIME_AGENT_H_ +#define SRC_INSPECTOR_RUNTIME_AGENT_H_ + +#include "node/inspector/protocol/NodeRuntime.h" +#include "v8.h" + +namespace node { +class Environment; + +namespace inspector { +namespace protocol { + +class RuntimeAgent : public NodeRuntime::Backend { + public: + explicit RuntimeAgent(Environment* env); + + void Wire(UberDispatcher* dispatcher); + + DispatchResponse notifyWhenWaitingForDisconnect(bool enabled) override; + + bool notifyWaitingForDisconnect(); + + private: + std::shared_ptr frontend_; + bool notify_when_waiting_for_disconnect_; + Environment* env_; +}; +} // namespace protocol +} // namespace inspector +} // namespace node + +#endif // SRC_INSPECTOR_RUNTIME_AGENT_H_ diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index d82e88d6123656..d8b5d01a285834 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -2,6 +2,7 @@ #include "inspector/main_thread_interface.h" #include "inspector/node_string.h" +#include "inspector/runtime_agent.h" #include "inspector/tracing_agent.h" #include "inspector/worker_agent.h" #include "inspector/worker_inspector.h" @@ -221,7 +222,8 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, std::unique_ptr delegate, std::shared_ptr main_thread_, bool prevent_shutdown) - : delegate_(std::move(delegate)), prevent_shutdown_(prevent_shutdown) { + : delegate_(std::move(delegate)), prevent_shutdown_(prevent_shutdown), + retaining_context_(false) { session_ = inspector->connect(1, this, StringView()); node_dispatcher_ = std::make_unique(this); tracing_agent_ = @@ -229,6 +231,8 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, tracing_agent_->Wire(node_dispatcher_.get()); worker_agent_ = std::make_unique(worker_manager); worker_agent_->Wire(node_dispatcher_.get()); + runtime_agent_ = std::make_unique(env); + runtime_agent_->Wire(node_dispatcher_.get()); } ~ChannelImpl() override { @@ -236,6 +240,8 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, tracing_agent_.reset(); // Dispose before the dispatchers worker_agent_->disable(); worker_agent_.reset(); // Dispose before the dispatchers + runtime_agent_->disable(); + runtime_agent_.reset(); // Dispose before the dispatchers } void dispatchProtocolMessage(const StringView& message) { @@ -264,6 +270,15 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, return prevent_shutdown_; } + bool notifyWaitingForDisconnect() { + retaining_context_ = runtime_agent_->notifyWaitingForDisconnect(); + return retaining_context_; + } + + bool retainingContext() { + return retaining_context_; + } + private: void sendResponse( int callId, @@ -303,12 +318,14 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, DCHECK(false); } + std::unique_ptr runtime_agent_; std::unique_ptr tracing_agent_; std::unique_ptr worker_agent_; std::unique_ptr delegate_; std::unique_ptr session_; std::unique_ptr node_dispatcher_; bool prevent_shutdown_; + bool retaining_context_; }; class InspectorTimer { @@ -511,7 +528,18 @@ class NodeInspectorClient : public V8InspectorClient { } void disconnectFrontend(int session_id) { - channels_.erase(session_id); + auto it = channels_.find(session_id); + if (it == channels_.end()) + return; + bool retaining_context = it->second->retainingContext(); + channels_.erase(it); + if (retaining_context) { + for (const auto& id_channel : channels_) { + if (id_channel.second->retainingContext()) + return; + } + contextDestroyed(env_->context()); + } } void dispatchMessageFromFrontend(int session_id, const StringView& message) { @@ -608,6 +636,15 @@ class NodeInspectorClient : public V8InspectorClient { return false; } + bool notifyWaitingForDisconnect() { + bool retaining_context = false; + for (const auto& id_channel : channels_) { + if (id_channel.second->notifyWaitingForDisconnect()) + retaining_context = true; + } + return retaining_context; + } + std::shared_ptr getThreadHandle() { if (interface_ == nullptr) { interface_.reset(new MainThreadInterface( @@ -774,9 +811,8 @@ void Agent::WaitForDisconnect() { fprintf(stderr, "Waiting for the debugger to disconnect...\n"); fflush(stderr); } - // TODO(addaleax): Maybe this should use an at-exit hook for the Environment - // or something similar? - client_->contextDestroyed(parent_env_->context()); + if (!client_->notifyWaitingForDisconnect()) + client_->contextDestroyed(parent_env_->context()); if (io_ != nullptr) { io_->StopAcceptingNewConnections(); client_->waitForIoShutdown(); diff --git a/test/common/inspector-helper.js b/test/common/inspector-helper.js index 4531ce5fccd380..b0c02b8fc52f49 100644 --- a/test/common/inspector-helper.js +++ b/test/common/inspector-helper.js @@ -191,6 +191,10 @@ class InspectorSession { } } + unprocessedNotifications() { + return this._unprocessedNotifications; + } + _sendMessage(message) { const msg = JSON.parse(JSON.stringify(message)); // Clone! msg.id = this._nextId++; diff --git a/test/parallel/test-inspector-waiting-for-disconnect.js b/test/parallel/test-inspector-waiting-for-disconnect.js new file mode 100644 index 00000000000000..4c2ebdb1daaaea --- /dev/null +++ b/test/parallel/test-inspector-waiting-for-disconnect.js @@ -0,0 +1,45 @@ +// Flags: --expose-internals +'use strict'; +const common = require('../common'); + +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const { NodeInstance } = require('../common/inspector-helper.js'); + +function mainContextDestroyed(notification) { + return notification.method === 'Runtime.executionContextDestroyed' && + notification.params.executionContextId === 1; +} + +async function runTest() { + const child = new NodeInstance(['--inspect-brk=0', '-e', 'process.exit(55)']); + const session = await child.connectInspectorSession(); + const oldStyleSession = await child.connectInspectorSession(); + await oldStyleSession.send([ + { method: 'Runtime.enable' }]); + await session.send([ + { method: 'Runtime.enable' }, + { method: 'NodeRuntime.notifyWhenWaitingForDisconnect', + params: { enabled: true } }, + { method: 'Runtime.runIfWaitingForDebugger' }]); + await session.waitForNotification((notification) => { + return notification.method === 'NodeRuntime.waitingForDisconnect'; + }); + const receivedExecutionContextDestroyed = + session.unprocessedNotifications().some(mainContextDestroyed); + if (receivedExecutionContextDestroyed) { + assert.fail('When NodeRuntime enabled, ' + + 'Runtime.executionContextDestroyed should not be sent'); + } + const { result: { value } } = await session.send({ + method: 'Runtime.evaluate', params: { expression: '42' } + }); + assert.strictEqual(value, 42); + await session.disconnect(); + await oldStyleSession.waitForNotification(mainContextDestroyed); + await oldStyleSession.disconnect(); + assert.strictEqual((await child.expectShutdown()).exitCode, 55); +} + +runTest();