From ab5f789e3f3f726702b86bc7b9661895780d4d12 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Sat, 8 Sep 2018 19:45:10 -0700 Subject: [PATCH] inspector: enable Inspector JS API in workers PR-URL: https://github.com/nodejs/node/pull/22769 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- lib/inspector.js | 2 +- lib/internal/util/inspector.js | 4 +-- lib/internal/worker.js | 4 ++- src/inspector/main_thread_interface.cc | 7 ---- src/inspector/main_thread_interface.h | 1 - src/inspector/tracing_agent.cc | 13 +++++--- src/inspector_agent.cc | 30 ++++++++++++++--- src/inspector_agent.h | 4 ++- src/node.cc | 2 +- src/node_worker.cc | 32 +++++++++++++++++-- src/node_worker.h | 3 +- test/common/README.md | 5 +++ test/common/index.js | 11 ++++--- test/parallel/test-bootstrap-modules.js | 2 +- .../test-inspector-port-zero-cluster.js | 1 + .../parallel/test-inspector-tracing-domain.js | 1 + .../test-trace-events-dynamic-enable.js | 1 + test/parallel/test-warn-sigprof.js | 2 ++ ...st-inspector-async-hook-setup-at-signal.js | 2 ++ test/sequential/test-inspector-contexts.js | 6 +++- .../sequential/test-inspector-port-cluster.js | 1 + 21 files changed, 99 insertions(+), 35 deletions(-) diff --git a/lib/inspector.js b/lib/inspector.js index 8827158757e126..6988eccf82c9ef 100644 --- a/lib/inspector.js +++ b/lib/inspector.js @@ -14,7 +14,7 @@ const util = require('util'); const { Connection, open, url } = process.binding('inspector'); const { originalConsole } = require('internal/process/per_thread'); -if (!Connection || !require('internal/worker').isMainThread) +if (!Connection) throw new ERR_INSPECTOR_NOT_AVAILABLE(); const connectionSymbol = Symbol('connectionProperty'); diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js index 3dd73415ded862..634d3302333584 100644 --- a/lib/internal/util/inspector.js +++ b/lib/internal/util/inspector.js @@ -1,8 +1,6 @@ 'use strict'; -// TODO(addaleax): Figure out how to integrate the inspector with workers. -const hasInspector = process.config.variables.v8_enable_inspector === 1 && - require('internal/worker').isMainThread; +const hasInspector = process.config.variables.v8_enable_inspector === 1; const inspector = hasInspector ? require('inspector') : undefined; let session; diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 402fc30b591d2d..4f797dd9e0094c 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -17,6 +17,7 @@ const { MessagePort, MessageChannel } = internalBinding('messaging'); const { handle_onclose } = internalBinding('symbols'); const { clearAsyncIdStack } = require('internal/async_hooks'); const { serializeError, deserializeError } = require('internal/error-serdes'); +const { pathToFileURL } = require('url'); util.inherits(MessagePort, EventEmitter); @@ -257,8 +258,9 @@ class Worker extends EventEmitter { } } + const url = options.eval ? null : pathToFileURL(filename); // Set up the C++ handle for the worker, as well as some internal wiring. - this[kHandle] = new WorkerImpl(); + this[kHandle] = new WorkerImpl(url); this[kHandle].onexit = (code) => this[kOnExit](code); this[kPort] = this[kHandle].messagePort; this[kPort].on('message', (data) => this[kOnMessage](data)); diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index d8ee737ce2fce0..d3f553caac8f9f 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -354,13 +354,6 @@ void MainThreadHandle::Reset() { main_thread_ = nullptr; } -Agent* MainThreadHandle::GetInspectorAgent() { - Mutex::ScopedLock scoped_lock(block_lock_); - if (main_thread_ == nullptr) - return nullptr; - return main_thread_->inspector_agent(); -} - std::unique_ptr MainThreadHandle::MakeDelegateThreadSafe( std::unique_ptr delegate) { diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index db79db43821a89..7092310e553c19 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -54,7 +54,6 @@ class MainThreadHandle : public std::enable_shared_from_this { return ++next_object_id_; } bool Post(std::unique_ptr request); - Agent* GetInspectorAgent(); std::unique_ptr MakeDelegateThreadSafe( std::unique_ptr delegate); bool Expired(); diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc index 6e962b289ab36f..fe69e6f863e2a6 100644 --- a/src/inspector/tracing_agent.cc +++ b/src/inspector/tracing_agent.cc @@ -74,11 +74,14 @@ DispatchResponse TracingAgent::start( if (categories_set.empty()) return DispatchResponse::Error("At least one category should be enabled"); - trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient( - categories_set, - std::unique_ptr( - new InspectorTraceWriter(frontend_.get())), - tracing::Agent::kIgnoreDefaultCategories); + auto* writer = env_->tracing_agent_writer(); + if (writer != nullptr) { + trace_writer_ = env_->tracing_agent_writer()->agent()->AddClient( + categories_set, + std::unique_ptr( + new InspectorTraceWriter(frontend_.get())), + tracing::Agent::kIgnoreDefaultCategories); + } return DispatchResponse::OK(); } diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 2eb9339d3e1641..63b92663532e06 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -190,6 +190,12 @@ static int StartDebugSignalHandler() { const int CONTEXT_GROUP_ID = 1; +std::string GetWorkerLabel(node::Environment* env) { + std::ostringstream result; + result << "Worker[" << env->thread_id() << "]"; + return result.str(); +} + class ChannelImpl final : public v8_inspector::V8Inspector::Channel, public protocol::FrontendChannel { public: @@ -393,10 +399,13 @@ bool IsFilePath(const std::string& path) { class NodeInspectorClient : public V8InspectorClient { public: - explicit NodeInspectorClient(node::Environment* env) : env_(env) { + explicit NodeInspectorClient(node::Environment* env, bool is_main) + : env_(env), is_main_(is_main) { client_ = V8Inspector::create(env->isolate(), this); // TODO(bnoordhuis) Make name configurable from src/node.cc. - ContextInfo info(GetHumanReadableProcessName()); + std::string name = + is_main_ ? GetHumanReadableProcessName() : GetWorkerLabel(env); + ContextInfo info(name); info.is_default = true; contextCreated(env->context(), info); } @@ -625,6 +634,7 @@ class NodeInspectorClient : public V8InspectorClient { } node::Environment* env_; + bool is_main_; bool running_nested_loop_ = false; std::unique_ptr client_; std::unordered_map> channels_; @@ -642,13 +652,23 @@ Agent::Agent(Environment* env) : parent_env_(env), debug_options_(env->options()->debug_options) {} -Agent::~Agent() = default; +Agent::~Agent() { + if (start_io_thread_async.data == this) { + start_io_thread_async.data = nullptr; + // This is global, will never get freed + uv_close(reinterpret_cast(&start_io_thread_async), nullptr); + } +} bool Agent::Start(const std::string& path, - std::shared_ptr options) { + std::shared_ptr options, + bool is_main) { + if (options == nullptr) { + options = std::make_shared(); + } path_ = path; debug_options_ = options; - client_ = std::make_shared(parent_env_); + client_ = std::make_shared(parent_env_, is_main); if (parent_env_->is_main_thread()) { CHECK_EQ(0, uv_async_init(parent_env_->event_loop(), &start_io_thread_async, diff --git a/src/inspector_agent.h b/src/inspector_agent.h index d9e2232dcc4d7b..79ae8d4cd99404 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -47,7 +47,9 @@ class Agent { ~Agent(); // Create client_, may create io_ if option enabled - bool Start(const std::string& path, std::shared_ptr options); + bool Start(const std::string& path, + std::shared_ptr options, + bool is_worker); // Stop and destroy io_ void Stop(); diff --git a/src/node.cc b/src/node.cc index 938f4c442b60e5..cd5173478170da 100644 --- a/src/node.cc +++ b/src/node.cc @@ -327,7 +327,7 @@ static struct { // right away on the websocket port and fails to bind/etc, this will return // false. return env->inspector_agent()->Start( - script_path == nullptr ? "" : script_path, options); + script_path == nullptr ? "" : script_path, options, true); } bool InspectorStarted(Environment* env) { diff --git a/src/node_worker.cc b/src/node_worker.cc index 80beac8aec973b..1a90e3a64fd843 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -35,10 +35,26 @@ namespace { uint64_t next_thread_id = 1; Mutex next_thread_id_mutex; +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR +void StartWorkerInspector(Environment* child, const std::string& url) { + child->inspector_agent()->Start(url, nullptr, false); +} + +void WaitForWorkerInspectorToStop(Environment* child) { + child->inspector_agent()->WaitForDisconnect(); + child->inspector_agent()->Stop(); +} + +#else +// No-ops +void StartWorkerInspector(Environment* child, const std::string& url) {} +void WaitForWorkerInspectorToStop(Environment* child) {} +#endif + } // anonymous namespace -Worker::Worker(Environment* env, Local wrap) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER) { +Worker::Worker(Environment* env, Local wrap, const std::string& url) + : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url) { // Generate a new thread id. { Mutex::ScopedLock next_thread_id_lock(next_thread_id_mutex); @@ -155,6 +171,7 @@ void Worker::Run() { env_->async_hooks()->pop_async_id(1); Debug(this, "Loaded environment for worker %llu", thread_id_); + StartWorkerInspector(env_.get(), url_); } { @@ -215,6 +232,7 @@ void Worker::Run() { env_->stop_sub_worker_contexts(); env_->RunCleanup(); RunAtExit(env_.get()); + WaitForWorkerInspectorToStop(env_.get()); { Mutex::ScopedLock stopped_lock(stopped_mutex_); @@ -351,7 +369,15 @@ void Worker::New(const FunctionCallbackInfo& args) { return; } - new Worker(env, args.This()); + std::string url; + // Argument might be a string or URL + if (args.Length() == 1 && !args[0]->IsNullOrUndefined()) { + Utf8Value value( + args.GetIsolate(), + args[0]->ToString(env->context()).FromMaybe(v8::Local())); + url.append(value.out(), value.length()); + } + new Worker(env, args.This(), url); } void Worker::StartThread(const FunctionCallbackInfo& args) { diff --git a/src/node_worker.h b/src/node_worker.h index 33df36e04ce670..8491ad221b6dde 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -12,7 +12,7 @@ namespace worker { // A worker thread, as represented in its parent thread. class Worker : public AsyncWrap { public: - Worker(Environment* env, v8::Local wrap); + Worker(Environment* env, v8::Local wrap, const std::string& url); ~Worker(); // Run the worker. This is only called from the worker thread. @@ -52,6 +52,7 @@ class Worker : public AsyncWrap { uv_loop_t loop_; DeleteFnPtr isolate_data_; DeleteFnPtr env_; + const std::string url_; v8::Isolate* isolate_ = nullptr; DeleteFnPtr array_buffer_allocator_; diff --git a/test/common/README.md b/test/common/README.md index 28d9bd04f0a8b6..0b3a7e9201e4f6 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -361,6 +361,11 @@ was disabled at compile time. Skip the rest of the tests in the current file when the Node.js executable was compiled with a pointer size smaller than 64 bits. +### skipIfWorker() + +Skip the rest of the tests in the current file when not running on a main +thread. + ## ArrayStream Module The `ArrayStream` module provides a simple `Stream` that pushes elements from diff --git a/test/common/index.js b/test/common/index.js index 5cc285c2cf59dc..f0f849e6afbacf 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -614,10 +614,6 @@ function skipIfInspectorDisabled() { if (process.config.variables.v8_enable_inspector === 0) { skip('V8 inspector is disabled'); } - if (!isMainThread) { - // TODO(addaleax): Fix me. - skip('V8 inspector is not available in Workers'); - } } function skipIf32Bits() { @@ -626,6 +622,12 @@ function skipIf32Bits() { } } +function skipIfWorker() { + if (!isMainThread) { + skip('This test only works on a main thread'); + } +} + function getArrayBufferViews(buf) { const { buffer, byteOffset, byteLength } = buf; @@ -744,6 +746,7 @@ module.exports = { skipIf32Bits, skipIfEslintMissing, skipIfInspectorDisabled, + skipIfWorker, get localhostIPv6() { return '::1'; }, diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 81563355d246ff..fdd651d3eeed84 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -11,4 +11,4 @@ const list = process.moduleLoadList.slice(); const assert = require('assert'); -assert(list.length <= 74, list); +assert(list.length <= 75, list); diff --git a/test/parallel/test-inspector-port-zero-cluster.js b/test/parallel/test-inspector-port-zero-cluster.js index e522056571d1c2..0330f1d50e6f70 100644 --- a/test/parallel/test-inspector-port-zero-cluster.js +++ b/test/parallel/test-inspector-port-zero-cluster.js @@ -3,6 +3,7 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +common.skipIfWorker(); // Assert that even when started with `--inspect=0` workers are assigned // consecutive (i.e. deterministically predictable) debug ports diff --git a/test/parallel/test-inspector-tracing-domain.js b/test/parallel/test-inspector-tracing-domain.js index 9c6fa471b5a39b..ecf15c5be97630 100644 --- a/test/parallel/test-inspector-tracing-domain.js +++ b/test/parallel/test-inspector-tracing-domain.js @@ -3,6 +3,7 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +common.skipIfWorker(); // https://github.com/nodejs/node/issues/22767 const assert = require('assert'); const { Session } = require('inspector'); diff --git a/test/parallel/test-trace-events-dynamic-enable.js b/test/parallel/test-trace-events-dynamic-enable.js index a32949f8ec5994..24a186c15a85c7 100644 --- a/test/parallel/test-trace-events-dynamic-enable.js +++ b/test/parallel/test-trace-events-dynamic-enable.js @@ -3,6 +3,7 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +common.skipIfWorker(); // https://github.com/nodejs/node/issues/22767 const assert = require('assert'); const { performance } = require('perf_hooks'); diff --git a/test/parallel/test-warn-sigprof.js b/test/parallel/test-warn-sigprof.js index e5335215d743b6..c3986a27be1703 100644 --- a/test/parallel/test-warn-sigprof.js +++ b/test/parallel/test-warn-sigprof.js @@ -10,6 +10,8 @@ common.skipIfInspectorDisabled(); if (common.isWindows) common.skip('test does not apply to Windows'); +common.skipIfWorker(); // Worker inspector never has a server running + common.expectWarning('Warning', 'process.on(SIGPROF) is reserved while debugging', common.noWarnCode); diff --git a/test/sequential/test-inspector-async-hook-setup-at-signal.js b/test/sequential/test-inspector-async-hook-setup-at-signal.js index 6f8ccfacb9964c..139678a1e5a493 100644 --- a/test/sequential/test-inspector-async-hook-setup-at-signal.js +++ b/test/sequential/test-inspector-async-hook-setup-at-signal.js @@ -6,6 +6,8 @@ common.skipIf32Bits(); const { NodeInstance } = require('../common/inspector-helper.js'); const assert = require('assert'); +common.skipIfWorker(); // Signal starts a server for a main thread inspector + const script = ` process._rawDebug('Waiting until a signal enables the inspector...'); let waiting = setInterval(waitUntilDebugged, 50); diff --git a/test/sequential/test-inspector-contexts.js b/test/sequential/test-inspector-contexts.js index d2285e82536326..793868e3bc072c 100644 --- a/test/sequential/test-inspector-contexts.js +++ b/test/sequential/test-inspector-contexts.js @@ -33,7 +33,11 @@ async function testContextCreatedAndDestroyed() { // the PID. strictEqual(name.includes(`[${process.pid}]`), true); } else { - strictEqual(`${process.argv0}[${process.pid}]`, name); + let expects = `${process.argv0}[${process.pid}]`; + if (!common.isMainThread) { + expects = `Worker[${require('worker_threads').threadId}]`; + } + strictEqual(expects, name); } strictEqual(origin, '', JSON.stringify(contextCreated)); diff --git a/test/sequential/test-inspector-port-cluster.js b/test/sequential/test-inspector-port-cluster.js index f7bebd926e9780..b58dbd83b6159a 100644 --- a/test/sequential/test-inspector-port-cluster.js +++ b/test/sequential/test-inspector-port-cluster.js @@ -3,6 +3,7 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +common.skipIfWorker(); const assert = require('assert'); const cluster = require('cluster');