Skip to content

Commit

Permalink
src: inspector context name = program title + pid
Browse files Browse the repository at this point in the history
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: #17087
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
  • Loading branch information
bnoordhuis committed Nov 20, 2017
1 parent 7dc35e9 commit f526deb
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 24 deletions.
3 changes: 2 additions & 1 deletion src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
13 changes: 1 addition & 12 deletions src/inspector_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,6 @@ using v8_inspector::StringView;
template<typename Transport>
using TransportAndIo = std::pair<Transport*, InspectorIo*>;

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;

Expand Down Expand Up @@ -484,7 +473,7 @@ std::vector<std::string> 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) {
Expand Down
13 changes: 5 additions & 8 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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");

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();
Expand Down
3 changes: 3 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename T, size_t N>
constexpr size_t arraysize(const T(&)[N]) { return N; }

Expand Down
12 changes: 12 additions & 0 deletions src/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
15 changes: 12 additions & 3 deletions test/sequential/test-inspector-contexts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down

0 comments on commit f526deb

Please sign in to comment.