From f526deb0407412e089382900e3fc4560eea3a938 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 20 Nov 2017 23:37:50 +0100 Subject: [PATCH] src: inspector context name = program title + pid Report (for example) "node[1337]" as the human-readable name rather than the more generic and less helpful "Node.js Main Context." While not perfect yet, it should be an improvement to people that debug multiple processes from DevTools, VS Code, etc. PR-URL: https://github.com/nodejs/node/pull/17087 Reviewed-By: Colin Ihrig Reviewed-By: Eugene Ostroukhov Reviewed-By: Timothy Gu --- src/inspector_agent.cc | 3 ++- src/inspector_io.cc | 13 +------------ src/node.cc | 11 ++++------- src/node_internals.h | 3 +++ src/util.cc | 12 ++++++++++++ test/sequential/test-inspector-contexts.js | 15 ++++++++++++--- 6 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 798b80df866..875c12efa02 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -302,7 +302,8 @@ class NodeInspectorClient : public V8InspectorClient { : env_(env), platform_(platform), terminated_(false), running_nested_loop_(false) { client_ = V8Inspector::create(env->isolate(), this); - contextCreated(env->context(), "Node.js Main Context"); + // TODO(bnoordhuis) Make name configurable from src/node.cc. + contextCreated(env->context(), GetHumanReadableProcessName()); } void runMessageLoopOnPause(int context_group_id) override { diff --git a/src/inspector_io.cc b/src/inspector_io.cc index a870c0a2634..538cbab3c9f 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -26,17 +26,6 @@ using v8_inspector::StringView; template using TransportAndIo = std::pair; -std::string GetProcessTitle() { - char title[2048]; - int err = uv_get_process_title(title, sizeof(title)); - if (err == 0) { - return title; - } else { - // Title is too long, or could not be retrieved. - return "Node.js"; - } -} - std::string ScriptPath(uv_loop_t* loop, const std::string& script_name) { std::string script_path; @@ -484,7 +473,7 @@ std::vector InspectorIoDelegate::GetTargetIds() { } std::string InspectorIoDelegate::GetTargetTitle(const std::string& id) { - return script_name_.empty() ? GetProcessTitle() : script_name_; + return script_name_.empty() ? GetHumanReadableProcessName() : script_name_; } std::string InspectorIoDelegate::GetTargetUrl(const std::string& id) { diff --git a/src/node.cc b/src/node.cc index 0bd9a44658b..e1b80c239c6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1653,14 +1653,11 @@ NO_RETURN void Assert(const char* const (*args)[4]) { auto message = (*args)[2]; auto function = (*args)[3]; - char exepath[256]; - size_t exepath_size = sizeof(exepath); - if (uv_exepath(exepath, &exepath_size)) - snprintf(exepath, sizeof(exepath), "node"); + char name[1024]; + GetHumanReadableProcessName(&name); - fprintf(stderr, "%s[%u]: %s:%s:%s%s Assertion `%s' failed.\n", - exepath, GetProcessId(), filename, linenum, - function, *function ? ":" : "", message); + fprintf(stderr, "%s: %s:%s:%s%s Assertion `%s' failed.\n", + name, filename, linenum, function, *function ? ":" : "", message); fflush(stderr); Abort(); diff --git a/src/node_internals.h b/src/node_internals.h index 7c4a0d9fa28..b7d032c68d1 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -251,6 +251,9 @@ void RegisterSignalHandler(int signal, uint32_t GetProcessId(); bool SafeGetenv(const char* key, std::string* text); +std::string GetHumanReadableProcessName(); +void GetHumanReadableProcessName(char (*name)[1024]); + template constexpr size_t arraysize(const T(&)[N]) { return N; } diff --git a/src/util.cc b/src/util.cc index 0fb897bc8e8..2aa9fb026ea 100644 --- a/src/util.cc +++ b/src/util.cc @@ -113,6 +113,18 @@ void LowMemoryNotification() { } } +std::string GetHumanReadableProcessName() { + char name[1024]; + GetHumanReadableProcessName(&name); + return name; +} + +void GetHumanReadableProcessName(char (*name)[1024]) { + char title[1024] = "Node.js"; + uv_get_process_title(title, sizeof(title)); + snprintf(*name, sizeof(*name), "%s[%u]", title, GetProcessId()); +} + uint32_t GetProcessId() { #ifdef _WIN32 return GetCurrentProcessId(); diff --git a/test/sequential/test-inspector-contexts.js b/test/sequential/test-inspector-contexts.js index 54acfab0d5c..c7db962f2af 100644 --- a/test/sequential/test-inspector-contexts.js +++ b/test/sequential/test-inspector-contexts.js @@ -23,9 +23,18 @@ async function testContextCreatedAndDestroyed() { session.post('Runtime.enable'); let contextCreated = await mainContextPromise; - strictEqual('Node.js Main Context', - contextCreated.params.context.name, - JSON.stringify(contextCreated)); + { + const { name } = contextCreated.params.context; + if (common.isSunOS || common.isWindows) { + // uv_get_process_title() is unimplemented on Solaris-likes, it returns + // an empy string. On the Windows CI buildbots it returns "Administrator: + // Windows PowerShell[42]" because of a GetConsoleTitle() quirk. Not much + // we can do about either, just verify that it contains the PID. + strictEqual(name.includes(`[${process.pid}]`), true); + } else { + strictEqual(`${process.argv0}[${process.pid}]`, name); + } + } const secondContextCreatedPromise = notificationPromise('Runtime.executionContextCreated');