From 46c16b3caec239b7e9eca829f3da3490730ebc43 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 7 Feb 2019 13:10:21 -0500 Subject: [PATCH 1/2] report: refactor report option validation PR-URL: https://github.com/nodejs/node/pull/25990 Reviewed-By: Richard Lau Reviewed-By: Anna Henningsen --- lib/internal/process/report.js | 133 +++++++++++++-------------------- 1 file changed, 50 insertions(+), 83 deletions(-) diff --git a/lib/internal/process/report.js b/lib/internal/process/report.js index 4ef03885ec64a7..3a0afaf76335dd 100644 --- a/lib/internal/process/report.js +++ b/lib/internal/process/report.js @@ -1,17 +1,13 @@ 'use strict'; - -const { emitExperimentalWarning } = require('internal/util'); +const { + convertToValidSignal, + emitExperimentalWarning +} = require('internal/util'); const { ERR_INVALID_ARG_TYPE, ERR_SYNTHETIC } = require('internal/errors').codes; -const REPORTEVENTS = 1; -const REPORTSIGNAL = 2; -const REPORTFILENAME = 3; -const REPORTPATH = 4; -const REPORTVERBOSE = 5; - // 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. @@ -35,68 +31,56 @@ let config = { 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); + const previousConfig = config; + const newConfig = {}; - // 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 = {}; + if (options === null || typeof options !== 'object') + 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); - }, + if (Array.isArray(options.events)) + newConfig.events = options.events.slice(); + else if (options.events === undefined) + newConfig.events = []; + else + throw new ERR_INVALID_ARG_TYPE('events', 'Array', options.events); + + if (typeof options.filename === 'string') + newConfig.filename = options.filename; + else if (options.filename === undefined) + newConfig.filename = ''; + else + throw new ERR_INVALID_ARG_TYPE('filename', 'string', options.filename); + + if (typeof options.path === 'string') + newConfig.path = options.path; + else if (options.path === undefined) + newConfig.path = ''; + else + throw new ERR_INVALID_ARG_TYPE('path', 'string', options.path); + if (typeof options.verbose === 'boolean') + newConfig.verbose = options.verbose; + else if (options.verbose === undefined) + newConfig.verbose = false; + else + throw new ERR_INVALID_ARG_TYPE('verbose', 'boolean', options.verbose); + if (typeof options.signal === 'string') + newConfig.signal = convertToValidSignal(options.signal); + else if (options.signal === undefined) + newConfig.signal = 'SIGUSR2'; + else + throw new ERR_INVALID_ARG_TYPE('signal', 'string', options.signal); + + if (previousConfig.signal) + process.removeListener(previousConfig.signal, handleSignal); + + if (newConfig.events.includes('signal')) + process.on(newConfig.signal, handleSignal); + + config = newConfig; + nr.syncConfig(config, true); + }, triggerReport(file, err) { emitExperimentalWarning('report'); if (err == null) { @@ -133,23 +117,6 @@ function handleSignal(signo) { nr.onUserSignal(signo); } -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); - return list; -} - module.exports = { config, handleSignal, From e154176b7cc397399ccb939b920b308f8d5cb874 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 7 Feb 2019 13:20:32 -0500 Subject: [PATCH 2/2] report: rename setDiagnosticReportOptions() setDiagnosticReportOptions() is a method on process.report, making the "DiagnosticReport" part redundant. Rename the function to setOptions(). PR-URL: https://github.com/nodejs/node/pull/25990 Reviewed-By: Richard Lau Reviewed-By: Anna Henningsen --- doc/api/process.md | 19 +++++++------------ doc/api/report.md | 10 +++++----- lib/internal/process/report.js | 2 +- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/doc/api/process.md b/doc/api/process.md index 7fb503d74f6505..b87223b9c41289 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -1687,7 +1687,7 @@ console.log(data); Additional documentation is available in the [report documentation][]. -### process.report.setDiagnosticReportOptions([options]); +### process.report.setOptions([options]); @@ -1712,23 +1712,18 @@ are shown below. ```js // Trigger a report on uncaught exceptions or fatal errors. -process.report.setDiagnosticReportOptions({ - events: ['exception', 'fatalerror'] -}); +process.report.setOptions({ events: ['exception', 'fatalerror'] }); // Change the default path and filename of the report. -process.report.setDiagnosticReportOptions({ - filename: 'foo.json', - path: '/home' -}); +process.report.setOptions({ filename: 'foo.json', path: '/home' }); // Produce the report onto stdout, when generated. Special meaning is attached // to `stdout` and `stderr`. Usage of these will result in report being written // to the associated standard streams. URLs are not supported. -process.report.setDiagnosticReportOptions({ filename: 'stdout' }); +process.report.setOptions({ filename: 'stdout' }); // Enable verbose option on report generation. -process.report.setDiagnosticReportOptions({ verbose: true }); +process.report.setOptions({ verbose: true }); ``` Signal based report generation is not supported on Windows. @@ -1742,8 +1737,8 @@ added: v11.8.0 * `filename` {string} Name of the file where the report is written. This should be a relative path, that will be appended to the directory specified in - `process.report.setDiagnosticReportOptions`, or the current working directory - of the Node.js process, if unspecified. + `process.report.setOptions`, or the current working directory of the Node.js + process, if unspecified. * `err` {Error} A custom error used for reporting the JavsScript stack. * Returns: {string} Returns the filename of the generated report. diff --git a/doc/api/report.md b/doc/api/report.md index 3937f88cc1be4a..89bb203691f7c9 100644 --- a/doc/api/report.md +++ b/doc/api/report.md @@ -445,10 +445,10 @@ times for the same Node.js process. ## Configuration Additional runtime configuration that influences the report generation -constraints are available using `setDiagnosticReportOptions()` API. +constraints are available using `setOptions()` API. ```js -process.report.setDiagnosticReportOptions({ +process.report.setOptions({ events: ['exception', 'fatalerror', 'signal'], signal: 'SIGUSR2', filename: 'myreport.json', @@ -481,13 +481,13 @@ pertinent to the report generation. Defaults to `false`. ```js // Trigger report only on uncaught exceptions. -process.report.setDiagnosticReportOptions({ events: ['exception'] }); +process.report.setOptions({ events: ['exception'] }); // Trigger report for both internal errors as well as external signal. -process.report.setDiagnosticReportOptions({ events: ['fatalerror', 'signal'] }); +process.report.setOptions({ events: ['fatalerror', 'signal'] }); // Change the default signal to `SIGQUIT` and enable it. -process.report.setDiagnosticReportOptions( +process.report.setOptions( { events: ['signal'], signal: 'SIGQUIT' }); ``` diff --git a/lib/internal/process/report.js b/lib/internal/process/report.js index 3a0afaf76335dd..95972773b18ecf 100644 --- a/lib/internal/process/report.js +++ b/lib/internal/process/report.js @@ -29,7 +29,7 @@ let config = { verbose: false }; const report = { - setDiagnosticReportOptions(options) { + setOptions(options) { emitExperimentalWarning('report'); const previousConfig = config; const newConfig = {};