From da03f0f680af96835ca348a73ff434922aad4037 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 15:03:20 +0100 Subject: [PATCH 1/6] report: remove `internalBinding('config').hasReport` The `setup()` method is only called when the `--experimental-report` option is set. `getOptionValue()` returns `undefined` when the flag is not defined, so the extra check inside of `setup()` is redundant. --- lib/internal/process/report.js | 265 ++++++++++++++++----------------- src/node_config.cc | 4 - 2 files changed, 132 insertions(+), 137 deletions(-) diff --git a/lib/internal/process/report.js b/lib/internal/process/report.js index 2d0d3f39214793..9b66526ce3dc91 100644 --- a/lib/internal/process/report.js +++ b/lib/internal/process/report.js @@ -11,153 +11,152 @@ exports.setup = function() { const REPORTFILENAME = 3; const REPORTPATH = 4; const REPORTVERBOSE = 5; - if (internalBinding('config').hasReport) { - // If report is enabled, extract the binding and - // wrap the APIs with thin layers, with some error checks. - // user options can come in from CLI / ENV / API. - // CLI and ENV is intercepted in C++ and the API call here (JS). - // So sync up with both sides as appropriate - initially from - // C++ to JS and from JS to C++ whenever the API is called. - // Some events are controlled purely from JS (signal | exception) - // and some from C++ (fatalerror) so this sync-up is essential for - // correct behavior and alignment with the supplied tunables. - const nr = internalBinding('report'); - // Keep it un-exposed; lest programs play with it - // leaving us with a lot of unwanted sanity checks. - let config = { - events: [], - signal: 'SIGUSR2', - filename: '', - path: '', - verbose: false - }; - const report = { - setDiagnosticReportOptions(options) { - emitExperimentalWarning('report'); - // Reuse the null and undefined checks. Save - // space when dealing with large number of arguments. - const list = parseOptions(options); + // If report is enabled, extract the binding and + // wrap the APIs with thin layers, with some error checks. + // user options can come in from CLI / ENV / API. + // CLI and ENV is intercepted in C++ and the API call here (JS). + // So sync up with both sides as appropriate - initially from + // C++ to JS and from JS to C++ whenever the API is called. + // Some events are controlled purely from JS (signal | exception) + // and some from C++ (fatalerror) so this sync-up is essential for + // correct behavior and alignment with the supplied tunables. + const nr = internalBinding('report'); - // Flush the stale entries from report, as - // we are refreshing it, items that the users did not - // touch may be hanging around stale otherwise. - config = {}; + // Keep it un-exposed; lest programs play with it + // leaving us with a lot of unwanted sanity checks. + let config = { + events: [], + signal: 'SIGUSR2', + filename: '', + path: '', + verbose: false + }; + const report = { + setDiagnosticReportOptions(options) { + emitExperimentalWarning('report'); + // Reuse the null and undefined checks. Save + // space when dealing with large number of arguments. + const list = parseOptions(options); - // The parseOption method returns an array that include - // the indices at which valid params are present. - list.forEach((i) => { - switch (i) { - case REPORTEVENTS: - if (Array.isArray(options.events)) - config.events = options.events; - else - throw new ERR_INVALID_ARG_TYPE('events', - 'Array', - options.events); - break; - case REPORTSIGNAL: - if (typeof options.signal !== 'string') { - throw new ERR_INVALID_ARG_TYPE('signal', - 'String', - options.signal); - } - process.removeListener(config.signal, handleSignal); - if (config.events.includes('signal')) - process.on(options.signal, handleSignal); - config.signal = options.signal; - break; - case REPORTFILENAME: - if (typeof options.filename !== 'string') { - throw new ERR_INVALID_ARG_TYPE('filename', - 'String', - options.filename); - } - config.filename = options.filename; - break; - case REPORTPATH: - if (typeof options.path !== 'string') - throw new ERR_INVALID_ARG_TYPE('path', 'String', options.path); - config.path = options.path; - break; - case REPORTVERBOSE: - if (typeof options.verbose !== 'string' && - typeof options.verbose !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE('verbose', - 'Booelan | String' + - ' (true|false|yes|no)', - options.verbose); - } - config.verbose = options.verbose; - break; - } - }); - // Upload this new config to C++ land - nr.syncConfig(config, true); - }, + // Flush the stale entries from report, as + // we are refreshing it, items that the users did not + // touch may be hanging around stale otherwise. + config = {}; + + // The parseOption method returns an array that include + // the indices at which valid params are present. + list.forEach((i) => { + switch (i) { + case REPORTEVENTS: + if (Array.isArray(options.events)) + config.events = options.events; + else + throw new ERR_INVALID_ARG_TYPE('events', + 'Array', + options.events); + break; + case REPORTSIGNAL: + if (typeof options.signal !== 'string') { + throw new ERR_INVALID_ARG_TYPE('signal', + 'String', + options.signal); + } + process.removeListener(config.signal, handleSignal); + if (config.events.includes('signal')) + process.on(options.signal, handleSignal); + config.signal = options.signal; + break; + case REPORTFILENAME: + if (typeof options.filename !== 'string') { + throw new ERR_INVALID_ARG_TYPE('filename', + 'String', + options.filename); + } + config.filename = options.filename; + break; + case REPORTPATH: + if (typeof options.path !== 'string') + throw new ERR_INVALID_ARG_TYPE('path', 'String', options.path); + config.path = options.path; + break; + case REPORTVERBOSE: + if (typeof options.verbose !== 'string' && + typeof options.verbose !== 'boolean') { + throw new ERR_INVALID_ARG_TYPE('verbose', + 'Booelan | String' + + ' (true|false|yes|no)', + options.verbose); + } + config.verbose = options.verbose; + break; + } + }); + // Upload this new config to C++ land + nr.syncConfig(config, true); + }, - triggerReport(file, err) { - emitExperimentalWarning('report'); - if (err == null) { - if (file == null) { - return nr.triggerReport(new ERR_SYNTHETIC( - 'JavaScript Callstack').stack); - } - if (typeof file !== 'string') - throw new ERR_INVALID_ARG_TYPE('file', 'String', file); - return nr.triggerReport(file, new ERR_SYNTHETIC( + triggerReport(file, err) { + emitExperimentalWarning('report'); + if (err == null) { + if (file == null) { + return nr.triggerReport(new ERR_SYNTHETIC( 'JavaScript Callstack').stack); } - if (typeof err !== 'object') - throw new ERR_INVALID_ARG_TYPE('err', 'Object', err); - if (file == null) - return nr.triggerReport(err.stack); if (typeof file !== 'string') throw new ERR_INVALID_ARG_TYPE('file', 'String', file); - return nr.triggerReport(file, err.stack); - }, - getReport(err) { - emitExperimentalWarning('report'); - if (err == null) { - return nr.getReport(new ERR_SYNTHETIC('JavaScript Callstack').stack); - } else if (typeof err !== 'object') { - throw new ERR_INVALID_ARG_TYPE('err', 'Objct', err); - } else { - return nr.getReport(err.stack); - } + return nr.triggerReport(file, new ERR_SYNTHETIC( + 'JavaScript Callstack').stack); } - }; + if (typeof err !== 'object') + throw new ERR_INVALID_ARG_TYPE('err', 'Object', err); + if (file == null) + return nr.triggerReport(err.stack); + if (typeof file !== 'string') + throw new ERR_INVALID_ARG_TYPE('file', 'String', file); + return nr.triggerReport(file, err.stack); + }, + getReport(err) { + emitExperimentalWarning('report'); + if (err == null) { + return nr.getReport(new ERR_SYNTHETIC('JavaScript Callstack').stack); + } else if (typeof err !== 'object') { + throw new ERR_INVALID_ARG_TYPE('err', 'Objct', err); + } else { + return nr.getReport(err.stack); + } + } + }; - // Download the CLI / ENV config into JS land. - nr.syncConfig(config, false); + // Download the CLI / ENV config into JS land. + nr.syncConfig(config, false); - function handleSignal(signo) { - if (typeof signo !== 'string') - signo = config.signal; - nr.onUserSignal(signo); - } + function handleSignal(signo) { + if (typeof signo !== 'string') + signo = config.signal; + nr.onUserSignal(signo); + } - if (config.events.includes('signal')) { - process.on(config.signal, handleSignal); - } + if (config.events.includes('signal')) { + process.on(config.signal, handleSignal); + } - function parseOptions(obj) { - const list = []; - if (obj == null) - return list; - if (obj.events != null) - list.push(REPORTEVENTS); - if (obj.signal != null) - list.push(REPORTSIGNAL); - if (obj.filename != null) - list.push(REPORTFILENAME); - if (obj.path != null) - list.push(REPORTPATH); - if (obj.verbose != null) - list.push(REPORTVERBOSE); + function parseOptions(obj) { + const list = []; + if (obj == null) return list; - } - process.report = report; + if (obj.events != null) + list.push(REPORTEVENTS); + if (obj.signal != null) + list.push(REPORTSIGNAL); + if (obj.filename != null) + list.push(REPORTFILENAME); + if (obj.path != null) + list.push(REPORTPATH); + if (obj.verbose != null) + list.push(REPORTVERBOSE); + return list; } + process.report = report; }; diff --git a/src/node_config.cc b/src/node_config.cc index af9403c1f9f854..65ad48f349eebd 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -69,10 +69,6 @@ static void Initialize(Local target, #endif // NODE_HAVE_I18N_SUPPORT -#if defined(NODE_REPORT) - READONLY_TRUE_PROPERTY(target, "hasReport"); -#endif // NODE_REPORT - if (env->abort_on_uncaught_exception()) READONLY_TRUE_PROPERTY(target, "shouldAbortOnUncaughtException"); From 768c1fdee870d660e923b1817a288e640ec1ae80 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 15:09:06 +0100 Subject: [PATCH 2/6] src: remove unnecessary `filename` variable --- src/node_errors.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index 3d607db2a88712..c7a449c8c50a4a 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -319,17 +319,16 @@ void OnFatalError(const char* location, const char* message) { } #ifdef NODE_REPORT Isolate* isolate = Isolate::GetCurrent(); - std::string filename; Environment* env = Environment::GetCurrent(isolate); if (env != nullptr) { std::shared_ptr options = env->isolate_data()->options(); if (options->report_on_fatalerror) { report::TriggerNodeReport( - isolate, env, message, __func__, filename, Local()); + isolate, env, message, __func__, "", Local()); } } else { report::TriggerNodeReport( - isolate, nullptr, message, __func__, filename, Local()); + isolate, nullptr, message, __func__, "", Local()); } #endif // NODE_REPORT fflush(stderr); From 7e0bed1672dc82987aa3b1ed153f0f6d879ec749 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 15:16:48 +0100 Subject: [PATCH 3/6] report: roll extra loop iteration in `PrintNativeStack()` --- src/node_report.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/node_report.cc b/src/node_report.cc index bb4c4e1a3a8e24..366ce73ea84af5 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -501,17 +501,13 @@ static void PrintNativeStack(JSONWriter* writer) { writer->json_arraystart("nativeStack"); int i; std::ostringstream buf; - for (i = 1; i < size - 1; i += 1) { + for (i = 1; i < size; i++) { void* frame = frames[i]; buf.str(""); buf << " [pc=" << frame << "] "; buf << sym_ctx->LookupSymbol(frame).Display().c_str(); writer->json_element(buf.str()); } - buf.str(""); - buf << " [pc=" << frames[i] << "] "; - buf << sym_ctx->LookupSymbol(frames[i]).Display().c_str(); - writer->json_element(buf.str()); writer->json_arrayend(); } From 3130406949c331500531a523d9d759b567eeb354 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 15:31:42 +0100 Subject: [PATCH 4/6] report: downgrade reinterpret_cast to static_cast --- src/node_report_utils.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_report_utils.cc b/src/node_report_utils.cc index e93f230d318a6d..75096a3d0ed701 100644 --- a/src/node_report_utils.cc +++ b/src/node_report_utils.cc @@ -112,7 +112,7 @@ void ReportPath(uv_handle_t* h, std::ostringstream& out) { void WalkHandle(uv_handle_t* h, void* arg) { std::string type; std::ostringstream data; - JSONWriter* writer = reinterpret_cast(arg); + JSONWriter* writer = static_cast(arg); uv_any_handle* handle = reinterpret_cast(h); // List all the types so we get a compile warning if we've missed one, From ec5ee3e3f43c601930aa79b817d1226208bfc04c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 15:35:17 +0100 Subject: [PATCH 5/6] report: use `uv_handle_type_name()` to get handle type --- src/node_report_utils.cc | 63 +++++----------------------------------- 1 file changed, 7 insertions(+), 56 deletions(-) diff --git a/src/node_report_utils.cc b/src/node_report_utils.cc index 75096a3d0ed701..e1cc580f34f982 100644 --- a/src/node_report_utils.cc +++ b/src/node_report_utils.cc @@ -110,7 +110,7 @@ void ReportPath(uv_handle_t* h, std::ostringstream& out) { // Utility function to walk libuv handles. void WalkHandle(uv_handle_t* h, void* arg) { - std::string type; + const char* type = uv_handle_type_name(h->type); std::ostringstream data; JSONWriter* writer = static_cast(arg); uv_any_handle* handle = reinterpret_cast(h); @@ -118,57 +118,20 @@ void WalkHandle(uv_handle_t* h, void* arg) { // List all the types so we get a compile warning if we've missed one, // (using default: supresses the compiler warning). switch (h->type) { - case UV_UNKNOWN_HANDLE: - type = "unknown"; - break; - case UV_ASYNC: - type = "async"; - break; - case UV_CHECK: - type = "check"; - break; - case UV_FS_EVENT: { - type = "fs_event"; - ReportPath(h, data); - break; - } - case UV_FS_POLL: { - type = "fs_poll"; + case UV_FS_EVENT: + case UV_FS_POLL: ReportPath(h, data); break; - } - case UV_HANDLE: - type = "handle"; - break; - case UV_IDLE: - type = "idle"; - break; - case UV_NAMED_PIPE: - type = "pipe"; - break; - case UV_POLL: - type = "poll"; - break; - case UV_PREPARE: - type = "prepare"; - break; - case UV_PROCESS: { - type = "process"; + case UV_PROCESS: data << "pid: " << handle->process.pid; break; - } - case UV_STREAM: - type = "stream"; - break; - case UV_TCP: { - type = "tcp"; + case UV_TCP: + case UV_UDP: ReportEndpoints(h, data); break; - } case UV_TIMER: { uint64_t due = handle->timer.timeout; uint64_t now = uv_now(handle->timer.loop); - type = "timer"; data << "repeat: " << uv_timer_get_repeat(&(handle->timer)); if (due > now) { data << ", timeout in: " << (due - now) << " ms"; @@ -179,22 +142,15 @@ void WalkHandle(uv_handle_t* h, void* arg) { } case UV_TTY: { int height, width, rc; - type = "tty"; rc = uv_tty_get_winsize(&(handle->tty), &width, &height); if (rc == 0) { data << "width: " << width << ", height: " << height; } break; } - case UV_UDP: { - type = "udp"; - ReportEndpoints(h, data); - break; - } case UV_SIGNAL: { // SIGWINCH is used by libuv so always appears. // See http://docs.libuv.org/en/v1.x/signal.html - type = "signal"; data << "signum: " << handle->signal.signum #ifndef _WIN32 << " (" << node::signo_string(handle->signal.signum) << ")" @@ -202,12 +158,7 @@ void WalkHandle(uv_handle_t* h, void* arg) { << ""; break; } - case UV_FILE: - type = "file"; - break; - // We shouldn't see "max" type - case UV_HANDLE_TYPE_MAX: - type = "max"; + default: break; } From d356f5bc1b329d6b2d48162408c651891a755d79 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 16:07:24 +0100 Subject: [PATCH 6/6] fixup! report: use `uv_handle_type_name()` to get handle type --- src/node_report_utils.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/node_report_utils.cc b/src/node_report_utils.cc index e1cc580f34f982..45f93e2f52d348 100644 --- a/src/node_report_utils.cc +++ b/src/node_report_utils.cc @@ -115,8 +115,6 @@ void WalkHandle(uv_handle_t* h, void* arg) { JSONWriter* writer = static_cast(arg); uv_any_handle* handle = reinterpret_cast(h); - // List all the types so we get a compile warning if we've missed one, - // (using default: supresses the compiler warning). switch (h->type) { case UV_FS_EVENT: case UV_FS_POLL: