From fe1ceb9842901bf357b3e58c05c2163461d1748a 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 | 13 +++++-------- src/node_internals.h | 3 +++ src/util.cc | 12 ++++++++++++ test/sequential/test-inspector-contexts.js | 15 ++++++++++++--- 6 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 3a3f4e0842ab52..fd1b0ed8f83ef4 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 a870c0a2634452..538cbab3c9fe84 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 e9a08f8165a3bf..1f1605e45faa93 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2034,14 +2034,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"); - - fprintf(stderr, "%s[%u]: %s:%s:%s%s Assertion `%s' failed.\n", - exepath, GetProcessId(), filename, linenum, - function, *function ? ":" : "", message); + char name[1024]; + GetHumanReadableProcessName(&name); + + 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 5056ace8664f70..b0802987fbd775 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -146,6 +146,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 0fb897bc8e891c..2aa9fb026ea315 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 54acfab0d5cace..c7db962f2af006 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');