diff options
author | Eugene Ostroukhov <eostroukhov@google.com> | 2018-09-08 19:45:10 -0700 |
---|---|---|
committer | Eugene Ostroukhov <eostroukhov@google.com> | 2018-09-17 09:49:53 -0700 |
commit | ab5f789e3f3f726702b86bc7b9661895780d4d12 (patch) | |
tree | 3c0e365e048ac7e3b0263c4dba86f59b8798e77a | |
parent | ab35194cb0297a85de5384b529f2cc03293d93bb (diff) | |
download | android-node-v8-ab5f789e3f3f726702b86bc7b9661895780d4d12.tar.gz android-node-v8-ab5f789e3f3f726702b86bc7b9661895780d4d12.tar.bz2 android-node-v8-ab5f789e3f3f726702b86bc7b9661895780d4d12.zip |
inspector: enable Inspector JS API in workers
PR-URL: https://github.com/nodejs/node/pull/22769
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
-rw-r--r-- | lib/inspector.js | 2 | ||||
-rw-r--r-- | lib/internal/util/inspector.js | 4 | ||||
-rw-r--r-- | lib/internal/worker.js | 4 | ||||
-rw-r--r-- | src/inspector/main_thread_interface.cc | 7 | ||||
-rw-r--r-- | src/inspector/main_thread_interface.h | 1 | ||||
-rw-r--r-- | src/inspector/tracing_agent.cc | 13 | ||||
-rw-r--r-- | src/inspector_agent.cc | 30 | ||||
-rw-r--r-- | src/inspector_agent.h | 4 | ||||
-rw-r--r-- | src/node.cc | 2 | ||||
-rw-r--r-- | src/node_worker.cc | 32 | ||||
-rw-r--r-- | src/node_worker.h | 3 | ||||
-rw-r--r-- | test/common/README.md | 5 | ||||
-rw-r--r-- | test/common/index.js | 11 | ||||
-rw-r--r-- | test/parallel/test-bootstrap-modules.js | 2 | ||||
-rw-r--r-- | test/parallel/test-inspector-port-zero-cluster.js | 1 | ||||
-rw-r--r-- | test/parallel/test-inspector-tracing-domain.js | 1 | ||||
-rw-r--r-- | test/parallel/test-trace-events-dynamic-enable.js | 1 | ||||
-rw-r--r-- | test/parallel/test-warn-sigprof.js | 2 | ||||
-rw-r--r-- | test/sequential/test-inspector-async-hook-setup-at-signal.js | 2 | ||||
-rw-r--r-- | test/sequential/test-inspector-contexts.js | 6 | ||||
-rw-r--r-- | test/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 8827158757..6988eccf82 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 3dd73415de..634d330233 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 402fc30b59..4f797dd9e0 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 d8ee737ce2..d3f553caac 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<InspectorSessionDelegate> MainThreadHandle::MakeDelegateThreadSafe( std::unique_ptr<InspectorSessionDelegate> delegate) { diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index db79db4382..7092310e55 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<MainThreadHandle> { return ++next_object_id_; } bool Post(std::unique_ptr<Request> request); - Agent* GetInspectorAgent(); std::unique_ptr<InspectorSessionDelegate> MakeDelegateThreadSafe( std::unique_ptr<InspectorSessionDelegate> delegate); bool Expired(); diff --git a/src/inspector/tracing_agent.cc b/src/inspector/tracing_agent.cc index 6e962b289a..fe69e6f863 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<InspectorTraceWriter>( - 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<InspectorTraceWriter>( + new InspectorTraceWriter(frontend_.get())), + tracing::Agent::kIgnoreDefaultCategories); + } return DispatchResponse::OK(); } diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 2eb9339d3e..63b9266353 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<V8Inspector> client_; std::unordered_map<int, std::unique_ptr<ChannelImpl>> 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<uv_handle_t*>(&start_io_thread_async), nullptr); + } +} bool Agent::Start(const std::string& path, - std::shared_ptr<DebugOptions> options) { + std::shared_ptr<DebugOptions> options, + bool is_main) { + if (options == nullptr) { + options = std::make_shared<DebugOptions>(); + } path_ = path; debug_options_ = options; - client_ = std::make_shared<NodeInspectorClient>(parent_env_); + client_ = std::make_shared<NodeInspectorClient>(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 d9e2232dcc..79ae8d4cd9 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<DebugOptions> options); + bool Start(const std::string& path, + std::shared_ptr<DebugOptions> options, + bool is_worker); // Stop and destroy io_ void Stop(); diff --git a/src/node.cc b/src/node.cc index 938f4c442b..cd51734781 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 80beac8aec..1a90e3a64f 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<Object> wrap) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER) { +Worker::Worker(Environment* env, Local<Object> 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<Value>& 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<v8::String>())); + url.append(value.out(), value.length()); + } + new Worker(env, args.This(), url); } void Worker::StartThread(const FunctionCallbackInfo<Value>& args) { diff --git a/src/node_worker.h b/src/node_worker.h index 33df36e04c..8491ad221b 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<v8::Object> wrap); + Worker(Environment* env, v8::Local<v8::Object> 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<IsolateData, FreeIsolateData> isolate_data_; DeleteFnPtr<Environment, FreeEnvironment> env_; + const std::string url_; v8::Isolate* isolate_ = nullptr; DeleteFnPtr<ArrayBufferAllocator, FreeArrayBufferAllocator> array_buffer_allocator_; diff --git a/test/common/README.md b/test/common/README.md index 28d9bd04f0..0b3a7e9201 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 5cc285c2cf..f0f849e6af 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 81563355d2..fdd651d3ee 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 e522056571..0330f1d50e 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 9c6fa471b5..ecf15c5be9 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 a32949f8ec..24a186c15a 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 e5335215d7..c3986a27be 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 6f8ccfacb9..139678a1e5 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 d2285e8253..793868e3bc 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 f7bebd926e..b58dbd83b6 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'); |