From a646a22d0f26314d44ce24b0dd200e4289297996 Mon Sep 17 00:00:00 2001
From: Debadree Chatterjee <debadree333@gmail.com>
Date: Mon, 6 Mar 2023 22:04:09 +0530
Subject: [PATCH] worker: add support for worker name in inspector and
 trace_events

Fixes: https://github.com/nodejs/node/issues/41589
PR-URL: https://github.com/nodejs/node/pull/46832
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
---
 doc/api/worker_threads.md                     |  7 +++++
 lib/internal/worker.js                        | 12 +++++--
 src/api/environment.cc                        |  8 ++++-
 src/inspector/worker_inspector.cc             | 23 ++++++++------
 src/inspector/worker_inspector.h              | 14 ++++-----
 src/inspector_agent.cc                        |  6 ++--
 src/inspector_agent.h                         |  2 +-
 src/node.h                                    |  6 ++++
 src/node_worker.cc                            | 21 +++++++++----
 src/node_worker.h                             |  3 ++
 test/fixtures/worker-name.js                  | 17 ++++++++++
 ...-trace-events-worker-metadata-with-name.js | 31 +++++++++++++++++++
 .../test-trace-events-worker-metadata.js      |  2 +-
 test/parallel/test-worker-name.js             | 22 +++++++++++++
 14 files changed, 144 insertions(+), 30 deletions(-)
 create mode 100644 test/fixtures/worker-name.js
 create mode 100644 test/parallel/test-trace-events-worker-metadata-with-name.js
 create mode 100644 test/parallel/test-worker-name.js

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 43f10ae489a9a0..674424f6f0bf90 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -906,6 +906,10 @@ if (isMainThread) {
 <!-- YAML
 added: v10.5.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/46832
+    description: Added support for a `name` option, which allows
+                 adding a name to worker title for debugging.
   - version: v14.9.0
     pr-url: https://github.com/nodejs/node/pull/34584
     description: The `filename` parameter can be a WHATWG `URL` object using
@@ -1004,6 +1008,9 @@ changes:
       used for generated code.
     * `stackSizeMb` {number} The default maximum stack size for the thread.
       Small values may lead to unusable Worker instances. **Default:** `4`.
+  * `name` {string} An optional `name` to be appended to the worker title
+    for debuggin/identification purposes, making the final title as
+    `[worker ${id}] ${name}`. **Default:** `''`.
 
 ### Event: `'error'`
 
diff --git a/lib/internal/worker.js b/lib/internal/worker.js
index 0304d7e4c7c6b8..f66b322b4fa56f 100644
--- a/lib/internal/worker.js
+++ b/lib/internal/worker.js
@@ -16,6 +16,7 @@ const {
   SafeArrayIterator,
   SafeMap,
   String,
+  StringPrototypeTrim,
   Symbol,
   SymbolFor,
   TypedArrayPrototypeFill,
@@ -57,7 +58,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 +189,19 @@ class Worker extends EventEmitter {
         options.env);
     }
 
+    let name = '';
+    if (options.name) {
+      validateString(options.name, 'options.name');
+      name = StringPrototypeTrim(options.name);
+    }
+
     // 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),
+                                   name);
     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 19c50398fa3da5..0b5c69d381bc6d 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -529,11 +529,17 @@ NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
     Environment* env,
     ThreadId thread_id,
     const char* url) {
+  return GetInspectorParentHandle(env, thread_id, url, "");
+}
+
+NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
+    Environment* env, ThreadId thread_id, const char* url, const char* name) {
   CHECK_NOT_NULL(env);
+  if (name == nullptr) name = "";
   CHECK_NE(thread_id.id, static_cast<uint64_t>(-1));
 #if HAVE_INSPECTOR
   return std::make_unique<InspectorParentHandleImpl>(
-      env->inspector_agent()->GetParentHandle(thread_id.id, url));
+      env->inspector_agent()->GetParentHandle(thread_id.id, url, name));
 #else
   return {};
 #endif
diff --git a/src/inspector/worker_inspector.cc b/src/inspector/worker_inspector.cc
index ff292ea4422b19..fd479181e9566c 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<node::inspector::MainThreadHandle> worker_thread,
-      bool waiting)
+      bool waiting,
+      const std::string& name)
       : id_(id),
-        info_(BuildWorkerTitle(id), url, worker_thread),
+        info_(BuildWorkerTitle(id, name), url, worker_thread),
         waiting_(waiting) {}
   void Call(MainThreadInterface* thread) override {
     auto manager = thread->inspector_agent()->GetWorkerManager();
@@ -24,8 +25,9 @@ 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& name) {
+    return "[worker " + std::to_string(id) + "]" +
+           (name == "" ? "" : " " + name);
   }
 
   uint64_t id_;
@@ -57,11 +59,13 @@ ParentInspectorHandle::ParentInspectorHandle(
     uint64_t id,
     const std::string& url,
     std::shared_ptr<MainThreadHandle> parent_thread,
-    bool wait_for_connect)
+    bool wait_for_connect,
+    const std::string& name)
     : id_(id),
       url_(url),
       parent_thread_(parent_thread),
-      wait_(wait_for_connect) {}
+      wait_(wait_for_connect),
+      name_(name) {}
 
 ParentInspectorHandle::~ParentInspectorHandle() {
   parent_thread_->Post(
@@ -71,7 +75,7 @@ ParentInspectorHandle::~ParentInspectorHandle() {
 void ParentInspectorHandle::WorkerStarted(
     std::shared_ptr<MainThreadHandle> worker_thread, bool waiting) {
   std::unique_ptr<Request> request(
-      new WorkerStartedRequest(id_, url_, worker_thread, waiting));
+      new WorkerStartedRequest(id_, url_, worker_thread, waiting, name_));
   parent_thread_->Post(std::move(request));
 }
 
@@ -97,9 +101,10 @@ void WorkerManager::WorkerStarted(uint64_t session_id,
 }
 
 std::unique_ptr<ParentInspectorHandle> WorkerManager::NewParentHandle(
-    uint64_t thread_id, const std::string& url) {
+    uint64_t thread_id, const std::string& url, const std::string& name) {
   bool wait = !delegates_waiting_on_start_.empty();
-  return std::make_unique<ParentInspectorHandle>(thread_id, url, thread_, wait);
+  return std::make_unique<ParentInspectorHandle>(
+      thread_id, url, thread_, wait, name);
 }
 
 void WorkerManager::RemoveAttachDelegate(int id) {
diff --git a/src/inspector/worker_inspector.h b/src/inspector/worker_inspector.h
index 540d98c742fe05..d3254d5aa0ebe4 100644
--- a/src/inspector/worker_inspector.h
+++ b/src/inspector/worker_inspector.h
@@ -56,14 +56,13 @@ class ParentInspectorHandle {
   ParentInspectorHandle(uint64_t id,
                         const std::string& url,
                         std::shared_ptr<MainThreadHandle> parent_thread,
-                        bool wait_for_connect);
+                        bool wait_for_connect,
+                        const std::string& name);
   ~ParentInspectorHandle();
   std::unique_ptr<ParentInspectorHandle> NewParentInspectorHandle(
-      uint64_t thread_id, const std::string& url) {
-    return std::make_unique<ParentInspectorHandle>(thread_id,
-                                                   url,
-                                                   parent_thread_,
-                                                   wait_);
+      uint64_t thread_id, const std::string& url, const std::string& name) {
+    return std::make_unique<ParentInspectorHandle>(
+        thread_id, url, parent_thread_, wait_, name);
   }
   void WorkerStarted(std::shared_ptr<MainThreadHandle> worker_thread,
                      bool waiting);
@@ -80,6 +79,7 @@ class ParentInspectorHandle {
   std::string url_;
   std::shared_ptr<MainThreadHandle> parent_thread_;
   bool wait_;
+  std::string name_;
 };
 
 class WorkerManager : public std::enable_shared_from_this<WorkerManager> {
@@ -88,7 +88,7 @@ class WorkerManager : public std::enable_shared_from_this<WorkerManager> {
                          : thread_(thread) {}
 
   std::unique_ptr<ParentInspectorHandle> NewParentHandle(
-      uint64_t thread_id, const std::string& url);
+      uint64_t thread_id, const std::string& url, const std::string& name);
   void WorkerStarted(uint64_t session_id, const WorkerInfo& info, bool waiting);
   void WorkerFinished(uint64_t session_id);
   std::unique_ptr<WorkerManagerEventHandle> SetAutoAttach(
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
index ad193d477a5a5c..02842c7e5e792c 100644
--- a/src/inspector_agent.cc
+++ b/src/inspector_agent.cc
@@ -952,7 +952,7 @@ void Agent::SetParentHandle(
 }
 
 std::unique_ptr<ParentInspectorHandle> Agent::GetParentHandle(
-    uint64_t thread_id, const std::string& url) {
+    uint64_t thread_id, const std::string& url, const std::string& name) {
   if (!parent_env_->should_create_inspector() && !client_) {
     ThrowUninitializedInspectorError(parent_env_);
     return std::unique_ptr<ParentInspectorHandle>{};
@@ -960,9 +960,9 @@ std::unique_ptr<ParentInspectorHandle> Agent::GetParentHandle(
 
   CHECK_NOT_NULL(client_);
   if (!parent_handle_) {
-    return client_->getWorkerManager()->NewParentHandle(thread_id, url);
+    return client_->getWorkerManager()->NewParentHandle(thread_id, url, name);
   } else {
-    return parent_handle_->NewParentInspectorHandle(thread_id, url);
+    return parent_handle_->NewParentInspectorHandle(thread_id, url, name);
   }
 }
 
diff --git a/src/inspector_agent.h b/src/inspector_agent.h
index 744e139df8f87a..01c5f70be6f860 100644
--- a/src/inspector_agent.h
+++ b/src/inspector_agent.h
@@ -82,7 +82,7 @@ class Agent {
 
   void SetParentHandle(std::unique_ptr<ParentInspectorHandle> parent_handle);
   std::unique_ptr<ParentInspectorHandle> GetParentHandle(
-      uint64_t thread_id, const std::string& url);
+      uint64_t thread_id, const std::string& url, const std::string& name);
 
   // 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 f16ac184c90838..5ca2f6d0dc89b7 100644
--- a/src/node.h
+++ b/src/node.h
@@ -696,6 +696,12 @@ NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
     ThreadId child_thread_id,
     const char* child_url);
 
+NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
+    Environment* parent_env,
+    ThreadId child_thread_id,
+    const char* child_url,
+    const char* name);
+
 struct StartExecutionCallbackInfo {
   v8::Local<v8::Object> process_object;
   v8::Local<v8::Function> native_require;
diff --git a/src/node_worker.cc b/src/node_worker.cc
index 66ab3dfd8ceb33..606e8cb74ba70f 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<Object> wrap,
                const std::string& url,
+               const std::string& name,
                std::shared_ptr<PerIsolateOptions> per_isolate_opts,
                std::vector<std::string>&& exec_argv,
                std::shared_ptr<KVStore> env_vars,
@@ -58,6 +59,7 @@ Worker::Worker(Environment* env,
       exec_argv_(exec_argv),
       platform_(env->isolate_data()->platform()),
       thread_id_(AllocateEnvironmentThreadId()),
+      name_(name),
       env_vars_(env_vars),
       snapshot_data_(snapshot_data) {
   Debug(this, "Creating new worker instance with thread id %llu",
@@ -82,8 +84,8 @@ Worker::Worker(Environment* env,
                 Number::New(env->isolate(), static_cast<double>(thread_id_.id)))
       .Check();
 
-  inspector_parent_handle_ = GetInspectorParentHandle(
-      env, thread_id_, url.c_str());
+  inspector_parent_handle_ =
+      GetInspectorParentHandle(env, thread_id_, url.c_str(), name.c_str());
 
   argv_ = std::vector<std::string>{env->argv()[0]};
   // Mark this Worker object as weak until we actually start the thread.
@@ -264,11 +266,10 @@ size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit,
 }
 
 void Worker::Run() {
-  std::string name = "WorkerThread ";
-  name += std::to_string(thread_id_.id);
+  std::string trace_name = "[worker " + std::to_string(thread_id_.id) + "]" +
+                           (name_ == "" ? "" : " " + name_);
   TRACE_EVENT_METADATA1(
-      "__metadata", "thread_name", "name",
-      TRACE_STR_COPY(name.c_str()));
+      "__metadata", "thread_name", "name", TRACE_STR_COPY(trace_name.c_str()));
   CHECK_NOT_NULL(platform_);
 
   Debug(this, "Creating isolate for worker with id %llu", thread_id_.id);
@@ -467,6 +468,7 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
   }
 
   std::string url;
+  std::string name;
   std::shared_ptr<PerIsolateOptions> per_isolate_opts = nullptr;
   std::shared_ptr<KVStore> env_vars = nullptr;
 
@@ -479,6 +481,12 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
     url.append(value.out(), value.length());
   }
 
+  if (!args[5]->IsNullOrUndefined()) {
+    Utf8Value value(
+        isolate, args[5]->ToString(env->context()).FromMaybe(Local<String>()));
+    name.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<Value>& args) {
   Worker* worker = new Worker(env,
                               args.This(),
                               url,
+                              name,
                               per_isolate_opts,
                               std::move(exec_argv_out),
                               env_vars,
diff --git a/src/node_worker.h b/src/node_worker.h
index dcb58d13e0e6f9..531e2b5287010f 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -30,6 +30,7 @@ class Worker : public AsyncWrap {
   Worker(Environment* env,
          v8::Local<v8::Object> wrap,
          const std::string& url,
+         const std::string& name,
          std::shared_ptr<PerIsolateOptions> per_isolate_opts,
          std::vector<std::string>&& exec_argv,
          std::shared_ptr<KVStore> env_vars,
@@ -99,6 +100,8 @@ class Worker : public AsyncWrap {
   ExitCode exit_code_ = ExitCode::kNoFailure;
   ThreadId thread_id_;
   uintptr_t stack_base_ = 0;
+  // Optional name used for debugging in inspector and trace events.
+  std::string name_;
 
   // Custom resource constraints:
   double resource_limits_[kTotalResourceLimitCount];
diff --git a/test/fixtures/worker-name.js b/test/fixtures/worker-name.js
new file mode 100644
index 00000000000000..45adea78f7e725
--- /dev/null
+++ b/test/fixtures/worker-name.js
@@ -0,0 +1,17 @@
+const { Session } = require('inspector');
+const { parentPort } = require('worker_threads');
+
+const session = new Session();
+
+parentPort.once('message', () => {}); // Prevent the worker from exiting.
+
+session.connectToMainThread();
+
+session.on(
+  'NodeWorker.attachedToWorker',
+  ({ params: { workerInfo } }) => {
+    // send the worker title to the main thread
+    parentPort.postMessage(workerInfo.title);
+  }
+);
+session.post('NodeWorker.enable', { waitForDebuggerOnStart: false });
diff --git a/test/parallel/test-trace-events-worker-metadata-with-name.js b/test/parallel/test-trace-events-worker-metadata-with-name.js
new file mode 100644
index 00000000000000..6c3a44f9566d9c
--- /dev/null
+++ b/test/parallel/test-trace-events-worker-metadata-with-name.js
@@ -0,0 +1,31 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const cp = require('child_process');
+const fs = require('fs');
+const { isMainThread } = require('worker_threads');
+
+if (isMainThread) {
+  const CODE = 'const { Worker } = require(\'worker_threads\'); ' +
+               `new Worker('${__filename.replace(/\\/g, '/')}', { name: 'foo' })`;
+  const FILE_NAME = 'node_trace.1.log';
+  const tmpdir = require('../common/tmpdir');
+  tmpdir.refresh();
+  process.chdir(tmpdir.path);
+
+  const proc = cp.spawn(process.execPath,
+                        [ '--trace-event-categories', 'node',
+                          '-e', CODE ]);
+  proc.once('exit', common.mustCall(() => {
+    assert(fs.existsSync(FILE_NAME));
+    fs.readFile(FILE_NAME, common.mustCall((err, data) => {
+      const traces = JSON.parse(data.toString()).traceEvents;
+      assert(traces.length > 0);
+      assert(traces.some((trace) =>
+        trace.cat === '__metadata' && trace.name === 'thread_name' &&
+          trace.args.name === '[worker 1] foo'));
+    }));
+  }));
+} else {
+  // Do nothing here.
+}
diff --git a/test/parallel/test-trace-events-worker-metadata.js b/test/parallel/test-trace-events-worker-metadata.js
index 4600ba3f7ec2fd..8b4d0be9c60713 100644
--- a/test/parallel/test-trace-events-worker-metadata.js
+++ b/test/parallel/test-trace-events-worker-metadata.js
@@ -23,7 +23,7 @@ if (isMainThread) {
       assert(traces.length > 0);
       assert(traces.some((trace) =>
         trace.cat === '__metadata' && trace.name === 'thread_name' &&
-          trace.args.name === 'WorkerThread 1'));
+          trace.args.name === '[worker 1]'));
     }));
   }));
 } else {
diff --git a/test/parallel/test-worker-name.js b/test/parallel/test-worker-name.js
new file mode 100644
index 00000000000000..952fcee0e05429
--- /dev/null
+++ b/test/parallel/test-worker-name.js
@@ -0,0 +1,22 @@
+'use strict';
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+
+common.skipIfInspectorDisabled();
+common.skipIfWorker(); // This test requires both main and worker threads.
+
+const assert = require('assert');
+const { Worker, isMainThread } = require('worker_threads');
+
+if (isMainThread) {
+  const name = 'Hello Thread';
+  const expectedTitle = `[worker 1] ${name}`;
+  const worker = new Worker(fixtures.path('worker-name.js'), {
+    name,
+  });
+  worker.once('message', common.mustCall((message) => {
+    assert.strictEqual(message, expectedTitle);
+    worker.postMessage('done');
+  }));
+}