From c977783875e6c59b5d089e9e6deec451a75e2d61 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sat, 25 Feb 2023 18:32:40 +0530 Subject: [PATCH 01/20] worker: add support for worker name in inspector and trace_events Fixes: https://github.com/nodejs/node/issues/41589 --- lib/internal/worker.js | 11 +++++++++-- src/api/environment.cc | 5 +++-- src/inspector/worker_inspector.cc | 21 ++++++++++++--------- src/inspector/worker_inspector.h | 11 +++++++---- src/inspector_agent.cc | 6 +++--- src/inspector_agent.h | 2 +- src/node.h | 3 ++- src/node_worker.cc | 11 ++++++++++- src/node_worker.h | 1 + test/cctest/test_environment.cc | 2 +- 10 files changed, 49 insertions(+), 24 deletions(-) diff --git a/lib/internal/worker.js b/lib/internal/worker.js index ccb4e7d12a5541..021c1481a76d69 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -57,7 +57,7 @@ const { const { deserializeError } = require('internal/error_serdes'); const { fileURLToPath, isURLInstance, pathToFileURL } = require('internal/url'); const { kEmptyObject } = require('internal/util'); -const { validateArray } = require('internal/validators'); +const { validateArray, validateString } = require('internal/validators'); const { ownsProcessState, @@ -188,12 +188,19 @@ class Worker extends EventEmitter { options.env); } + let title_prefix = ''; + if (options.titlePrefix) { + validateString(options.titlePrefix, 'options.titlePrefix'); + title_prefix = options.titlePrefix; + } + // Set up the C++ handle for the worker, as well as some internal wiring. this[kHandle] = new WorkerImpl(url, env === process.env ? null : env, options.execArgv, parseResourceLimits(options.resourceLimits), - !!(options.trackUnmanagedFds ?? true)); + !!(options.trackUnmanagedFds ?? true), + title_prefix); if (this[kHandle].invalidExecArgv) { throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv); } diff --git a/src/api/environment.cc b/src/api/environment.cc index 3d3f864d4e956c..9bcb458dfa37bc 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -508,12 +508,13 @@ void FreeEnvironment(Environment* env) { NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* env, ThreadId thread_id, - const char* url) { + const char* url, + const char* title_prefix) { CHECK_NOT_NULL(env); CHECK_NE(thread_id.id, static_cast(-1)); #if HAVE_INSPECTOR return std::make_unique( - env->inspector_agent()->GetParentHandle(thread_id.id, url)); + env->inspector_agent()->GetParentHandle(thread_id.id, url, title_prefix)); #else return {}; #endif diff --git a/src/inspector/worker_inspector.cc b/src/inspector/worker_inspector.cc index ff292ea4422b19..bbcd2612ec0770 100644 --- a/src/inspector/worker_inspector.cc +++ b/src/inspector/worker_inspector.cc @@ -14,9 +14,10 @@ class WorkerStartedRequest : public Request { uint64_t id, const std::string& url, std::shared_ptr worker_thread, - bool waiting) + bool waiting, + const std::string& title_prefix) : id_(id), - info_(BuildWorkerTitle(id), url, worker_thread), + info_(BuildWorkerTitle(id, title_prefix), url, worker_thread), waiting_(waiting) {} void Call(MainThreadInterface* thread) override { auto manager = thread->inspector_agent()->GetWorkerManager(); @@ -24,8 +25,8 @@ class WorkerStartedRequest : public Request { } private: - static std::string BuildWorkerTitle(int id) { - return "Worker " + std::to_string(id); + static std::string BuildWorkerTitle(int id, const std::string& title_prefix) { + return title_prefix + "Worker " + std::to_string(id); } uint64_t id_; @@ -57,11 +58,13 @@ ParentInspectorHandle::ParentInspectorHandle( uint64_t id, const std::string& url, std::shared_ptr parent_thread, - bool wait_for_connect) + bool wait_for_connect, + const std::string& title_prefix) : id_(id), url_(url), parent_thread_(parent_thread), - wait_(wait_for_connect) {} + wait_(wait_for_connect), + title_prefix_(title_prefix) {} ParentInspectorHandle::~ParentInspectorHandle() { parent_thread_->Post( @@ -71,7 +74,7 @@ ParentInspectorHandle::~ParentInspectorHandle() { void ParentInspectorHandle::WorkerStarted( std::shared_ptr worker_thread, bool waiting) { std::unique_ptr request( - new WorkerStartedRequest(id_, url_, worker_thread, waiting)); + new WorkerStartedRequest(id_, url_, worker_thread, waiting, title_prefix_)); parent_thread_->Post(std::move(request)); } @@ -97,9 +100,9 @@ void WorkerManager::WorkerStarted(uint64_t session_id, } std::unique_ptr WorkerManager::NewParentHandle( - uint64_t thread_id, const std::string& url) { + uint64_t thread_id, const std::string& url, const std::string& title_prefix) { bool wait = !delegates_waiting_on_start_.empty(); - return std::make_unique(thread_id, url, thread_, wait); + return std::make_unique(thread_id, url, thread_, wait, title_prefix); } void WorkerManager::RemoveAttachDelegate(int id) { diff --git a/src/inspector/worker_inspector.h b/src/inspector/worker_inspector.h index 540d98c742fe05..f422807c0f8997 100644 --- a/src/inspector/worker_inspector.h +++ b/src/inspector/worker_inspector.h @@ -56,14 +56,16 @@ class ParentInspectorHandle { ParentInspectorHandle(uint64_t id, const std::string& url, std::shared_ptr parent_thread, - bool wait_for_connect); + bool wait_for_connect, + const std::string& title_prefix); ~ParentInspectorHandle(); std::unique_ptr NewParentInspectorHandle( - uint64_t thread_id, const std::string& url) { + uint64_t thread_id, const std::string& url, const std::string& title_prefix) { return std::make_unique(thread_id, url, parent_thread_, - wait_); + wait_, + title_prefix); } void WorkerStarted(std::shared_ptr worker_thread, bool waiting); @@ -80,6 +82,7 @@ class ParentInspectorHandle { std::string url_; std::shared_ptr parent_thread_; bool wait_; + std::string title_prefix_; }; class WorkerManager : public std::enable_shared_from_this { @@ -88,7 +91,7 @@ class WorkerManager : public std::enable_shared_from_this { : thread_(thread) {} std::unique_ptr NewParentHandle( - uint64_t thread_id, const std::string& url); + uint64_t thread_id, const std::string& url, const std::string& title_prefix); void WorkerStarted(uint64_t session_id, const WorkerInfo& info, bool waiting); void WorkerFinished(uint64_t session_id); std::unique_ptr SetAutoAttach( diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index ad193d477a5a5c..d984537573e6f4 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -952,7 +952,7 @@ void Agent::SetParentHandle( } std::unique_ptr Agent::GetParentHandle( - uint64_t thread_id, const std::string& url) { + uint64_t thread_id, const std::string& url, const std::string& title_prefix) { if (!parent_env_->should_create_inspector() && !client_) { ThrowUninitializedInspectorError(parent_env_); return std::unique_ptr{}; @@ -960,9 +960,9 @@ std::unique_ptr Agent::GetParentHandle( CHECK_NOT_NULL(client_); if (!parent_handle_) { - return client_->getWorkerManager()->NewParentHandle(thread_id, url); + return client_->getWorkerManager()->NewParentHandle(thread_id, url, title_prefix); } else { - return parent_handle_->NewParentInspectorHandle(thread_id, url); + return parent_handle_->NewParentInspectorHandle(thread_id, url, title_prefix); } } diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 744e139df8f87a..019173e5b1b405 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -82,7 +82,7 @@ class Agent { void SetParentHandle(std::unique_ptr parent_handle); std::unique_ptr GetParentHandle( - uint64_t thread_id, const std::string& url); + uint64_t thread_id, const std::string& url, const std::string& title_prefix); // Called to create inspector sessions that can be used from the same thread. // The inspector responds by using the delegate to send messages back. diff --git a/src/node.h b/src/node.h index 635e5349f4ee88..4482e7dc5753d3 100644 --- a/src/node.h +++ b/src/node.h @@ -677,7 +677,8 @@ NODE_EXTERN Environment* CreateEnvironment( NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* parent_env, ThreadId child_thread_id, - const char* child_url); + const char* child_url, + const char* title_prefix); struct StartExecutionCallbackInfo { v8::Local process_object; diff --git a/src/node_worker.cc b/src/node_worker.cc index 66ab3dfd8ceb33..14644d431f2875 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -49,6 +49,7 @@ constexpr double kMB = 1024 * 1024; Worker::Worker(Environment* env, Local wrap, const std::string& url, + const std::string& title_prefix, std::shared_ptr per_isolate_opts, std::vector&& exec_argv, std::shared_ptr env_vars, @@ -83,7 +84,7 @@ Worker::Worker(Environment* env, .Check(); inspector_parent_handle_ = GetInspectorParentHandle( - env, thread_id_, url.c_str()); + env, thread_id_, url.c_str(), title_prefix.c_str()); argv_ = std::vector{env->argv()[0]}; // Mark this Worker object as weak until we actually start the thread. @@ -467,6 +468,7 @@ void Worker::New(const FunctionCallbackInfo& args) { } std::string url; + std::string title_prefix; std::shared_ptr per_isolate_opts = nullptr; std::shared_ptr env_vars = nullptr; @@ -479,6 +481,12 @@ void Worker::New(const FunctionCallbackInfo& args) { url.append(value.out(), value.length()); } + if (!args[5]->IsNullOrUndefined()) { + Utf8Value value( + isolate, args[5]->ToString(env->context()).FromMaybe(Local())); + title_prefix.append(value.out(), value.length()); + } + if (args[1]->IsNull()) { // Means worker.env = { ...process.env }. env_vars = env->env_vars()->Clone(isolate); @@ -589,6 +597,7 @@ void Worker::New(const FunctionCallbackInfo& args) { Worker* worker = new Worker(env, args.This(), url, + title_prefix, per_isolate_opts, std::move(exec_argv_out), env_vars, diff --git a/src/node_worker.h b/src/node_worker.h index dcb58d13e0e6f9..266d5ed09d01df 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -30,6 +30,7 @@ class Worker : public AsyncWrap { Worker(Environment* env, v8::Local wrap, const std::string& url, + const std::string& title_prefix, std::shared_ptr per_isolate_opts, std::vector&& exec_argv, std::shared_ptr env_vars, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 547c8ddbffe243..787564f4036c6c 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -449,7 +449,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { ChildEnvironmentData data; data.thread_id = node::AllocateEnvironmentThreadId(); data.inspector_parent_handle = - GetInspectorParentHandle(*env, data.thread_id, "file:///embedded.js"); + GetInspectorParentHandle(*env, data.thread_id, "file:///embedded.js", ""); CHECK(data.inspector_parent_handle); data.platform = GetMultiIsolatePlatform(*env); CHECK_NOT_NULL(data.platform); From 35811f5f59ffbef2b22e932be60b4b9efa92b62c Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sun, 26 Feb 2023 11:53:24 +0530 Subject: [PATCH 02/20] fixup! remove breaking change --- src/api/environment.cc | 14 ++++++++++++++ src/node.h | 5 +++++ test/cctest/test_environment.cc | 2 +- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 9bcb458dfa37bc..c562b6c2e01734 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -505,6 +505,20 @@ void FreeEnvironment(Environment* env) { delete env; } +NODE_EXTERN std::unique_ptr GetInspectorParentHandle( + Environment* env, + ThreadId thread_id, + const char* url) { + CHECK_NOT_NULL(env); + CHECK_NE(thread_id.id, static_cast(-1)); +#if HAVE_INSPECTOR + return std::make_unique( + env->inspector_agent()->GetParentHandle(thread_id.id, url, "")); +#else + return {}; +#endif +} + NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* env, ThreadId thread_id, diff --git a/src/node.h b/src/node.h index 4482e7dc5753d3..526b7f7b50703a 100644 --- a/src/node.h +++ b/src/node.h @@ -674,6 +674,11 @@ NODE_EXTERN Environment* CreateEnvironment( // Environment, together with the inspector handle. // This method should not be called while the parent Environment is active // on another thread. +NODE_EXTERN std::unique_ptr GetInspectorParentHandle( + Environment* parent_env, + ThreadId child_thread_id, + const char* child_url); + NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* parent_env, ThreadId child_thread_id, diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 787564f4036c6c..547c8ddbffe243 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -449,7 +449,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { ChildEnvironmentData data; data.thread_id = node::AllocateEnvironmentThreadId(); data.inspector_parent_handle = - GetInspectorParentHandle(*env, data.thread_id, "file:///embedded.js", ""); + GetInspectorParentHandle(*env, data.thread_id, "file:///embedded.js"); CHECK(data.inspector_parent_handle); data.platform = GetMultiIsolatePlatform(*env); CHECK_NOT_NULL(data.platform); From 2680a3e09e4808cf6af0cbae9ecedbd982b2029f Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sun, 26 Feb 2023 13:19:13 +0530 Subject: [PATCH 03/20] fixup! add a test --- test/parallel/test-worker-title-prefix.js | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 test/parallel/test-worker-title-prefix.js diff --git a/test/parallel/test-worker-title-prefix.js b/test/parallel/test-worker-title-prefix.js new file mode 100644 index 00000000000000..5ba3b5945a2520 --- /dev/null +++ b/test/parallel/test-worker-title-prefix.js @@ -0,0 +1,25 @@ +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); +const assert = require('assert'); +const { Worker, isMainThread } = require('worker_threads'); + +const { Session } = require('inspector'); + +if (isMainThread) { + const titlePrefix = 'Hello Thread '; + new Worker(__filename, { + workerData: 'Test Worker', + titlePrefix, + }); + + const session = new Session(); + session.connect(); + session.on('NodeWorker.attachedToWorker', ({ params: { workerInfo } }) => { + const id = workerInfo.id; + const expectedTitle = `${titlePrefix}Worker ${id}`; + assert.strictEqual(workerInfo.title, expectedTitle); + }); + session.post('NodeWorker.enable', { waitForDebuggerOnStart: false }); +} From b6738e1d359e1511e16361bda4b3ad7dc37f8cf3 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sun, 26 Feb 2023 13:30:27 +0530 Subject: [PATCH 04/20] fixup! add doc --- doc/api/worker_threads.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 43f10ae489a9a0..116566e74ee48c 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -906,6 +906,10 @@ if (isMainThread) {