Skip to content

Commit

Permalink
inspector: --debug* deprecation and invalidation
Browse files Browse the repository at this point in the history
PR-URL: #12949
Fixes: #12364
Fixes: #12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
  • Loading branch information
refack committed May 29, 2017
1 parent dbbe1fa commit 16689e3
Show file tree
Hide file tree
Showing 10 changed files with 180 additions and 36 deletions.
14 changes: 14 additions & 0 deletions lib/internal/bootstrap_node.js
Expand Up @@ -65,6 +65,20 @@
});
process.argv[0] = process.execPath;

// Handle `--debug*` deprecation and invalidation
if (process._invalidDebug) {
process.emitWarning(
'`node --debug` and `node --debug-brk` are invalid. ' +
'Please use `node --inspect` or `node --inspect-brk` instead.',
'DeprecationWarning', 'DEP0062', startup, true);
process.exit(9);
} else if (process._deprecatedDebugBrk) {
process.emitWarning(
'`node --inspect --debug-brk` is deprecated. ' +
'Please use `node --inspect-brk` instead.',
'DeprecationWarning', 'DEP0062', startup, true);
}

// There are various modes that Node can run in. The most common two
// are running from a script and running the REPL - but there are a few
// others like the debugger or running --eval arguments. Here we decide
Expand Down
5 changes: 3 additions & 2 deletions lib/internal/process/warning.js
Expand Up @@ -111,7 +111,7 @@ function setupProcessWarnings() {
// process.emitWarning(error)
// process.emitWarning(str[, type[, code]][, ctor])
// process.emitWarning(str[, options])
process.emitWarning = function(warning, type, code, ctor) {
process.emitWarning = function(warning, type, code, ctor, now) {
const errors = lazyErrors();
var detail;
if (type !== null && typeof type === 'object' && !Array.isArray(type)) {
Expand Down Expand Up @@ -150,6 +150,7 @@ function setupProcessWarnings() {
if (process.throwDeprecation)
throw warning;
}
process.nextTick(doEmitWarning(warning));
if (now) process.emit('warning', warning);
else process.nextTick(doEmitWarning(warning));
};
}
4 changes: 2 additions & 2 deletions lib/module.js
Expand Up @@ -537,7 +537,7 @@ Module.prototype._compile = function(content, filename) {
});

var inspectorWrapper = null;
if (process._debugWaitConnect && process._eval == null) {
if (process._breakFirstLine && process._eval == null) {
if (!resolvedArgv) {
// we enter the repl if we're not given a filename argument.
if (process.argv[1]) {
Expand All @@ -549,7 +549,7 @@ Module.prototype._compile = function(content, filename) {

// Set breakpoint on module start
if (filename === resolvedArgv) {
delete process._debugWaitConnect;
delete process._breakFirstLine;
inspectorWrapper = process.binding('inspector').callAndPauseOnStart;
if (!inspectorWrapper) {
const Debug = vm.runInDebugContext('Debug');
Expand Down
22 changes: 21 additions & 1 deletion src/node.cc
Expand Up @@ -3391,9 +3391,29 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "traceDeprecation", True(env->isolate()));
}

// TODO(refack): move the following 4 to `node_config`
// --inspect
if (debug_options.inspector_enabled()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_inspectorEnbale", True(env->isolate()));

This comment has been minimized.

Copy link
@sam-github

sam-github Jun 5, 2017

Contributor

Note that when SIGUSR1 is used to start the inspector, this property won't change. So, it does literally reflect --inspect, but it doesn't reflect the current state of whether the inspector port is open or not. Not sure its intention, but FYI.

}

// --inspect-brk
if (debug_options.wait_for_connect()) {
READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
READONLY_DONT_ENUM_PROPERTY(process,
"_breakFirstLine", True(env->isolate()));
}

// --inspect --debug-brk
if (debug_options.deprecated_invocation()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_deprecatedDebugBrk", True(env->isolate()));
}

// --debug or, --debug-brk without --inspect
if (debug_options.invalid_invocation()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_invalidDebug", True(env->isolate()));
}

// --security-revert flags
Expand Down
37 changes: 20 additions & 17 deletions src/node_debug_options.cc
Expand Up @@ -8,9 +8,7 @@
namespace node {

namespace {
#if HAVE_INSPECTOR
const int default_inspector_port = 9229;
#endif // HAVE_INSPECTOR

inline std::string remove_brackets(const std::string& host) {
if (!host.empty() && host.front() == '[' && host.back() == ']')
Expand Down Expand Up @@ -56,14 +54,12 @@ std::pair<std::string, int> split_host_port(const std::string& arg) {
} // namespace

DebugOptions::DebugOptions() :
#if HAVE_INSPECTOR
inspector_enabled_(false),
#endif // HAVE_INSPECTOR
wait_connect_(false), http_enabled_(false),
deprecated_debug_(false),
break_first_line_(false),
host_name_("127.0.0.1"), port_(-1) { }

bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
bool enable_inspector = false;
bool has_argument = false;
std::string option_name;
std::string argument;
Expand All @@ -81,11 +77,21 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
argument.clear();
}

// Note that --debug-port and --debug-brk in conjuction with --inspect
// work but are undocumented.
// --debug is no longer valid.
// Ref: https://github.com/nodejs/node/issues/12630
// Ref: https://github.com/nodejs/node/pull/12949
if (option_name == "--inspect") {
enable_inspector = true;
inspector_enabled_ = true;
} else if (option_name == "--debug") {
deprecated_debug_ = true;
} else if (option_name == "--inspect-brk") {
enable_inspector = true;
wait_connect_ = true;
inspector_enabled_ = true;
break_first_line_ = true;
} else if (option_name == "--debug-brk") {
break_first_line_ = true;
deprecated_debug_ = true;
} else if (option_name == "--debug-port" ||
option_name == "--inspect-port") {
if (!has_argument) {
Expand All @@ -97,15 +103,14 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
return false;
}

if (enable_inspector) {
#if HAVE_INSPECTOR
inspector_enabled_ = true;
#else
#if !HAVE_INSPECTOR
if (inspector_enabled_) {
fprintf(stderr,
"Inspector support is not available with this Node.js build\n");
return false;
#endif
}
inspector_enabled_ = false;
return false;
#endif

// argument can be specified for *any* option to specify host:port
if (has_argument) {
Expand All @@ -124,9 +129,7 @@ bool DebugOptions::ParseOption(const char* argv0, const std::string& option) {
int DebugOptions::port() const {
int port = port_;
if (port < 0) {
#if HAVE_INSPECTOR
port = default_inspector_port;
#endif // HAVE_INSPECTOR
}
return port;
}
Expand Down
23 changes: 11 additions & 12 deletions src/node_debug_options.h
Expand Up @@ -10,25 +10,24 @@ class DebugOptions {
public:
DebugOptions();
bool ParseOption(const char* argv0, const std::string& option);
bool inspector_enabled() const {
#if HAVE_INSPECTOR
return inspector_enabled_;
#else
return false;
#endif // HAVE_INSPECTOR
bool inspector_enabled() const { return inspector_enabled_; }
bool deprecated_invocation() const {
return deprecated_debug_ &&
inspector_enabled_ &&
break_first_line_;
}
bool ToolsServerEnabled();
bool wait_for_connect() const { return wait_connect_; }
bool invalid_invocation() const {
return deprecated_debug_ && !inspector_enabled_;
}
bool wait_for_connect() const { return break_first_line_; }
std::string host_name() const { return host_name_; }
int port() const;
void set_port(int port) { port_ = port; }

private:
#if HAVE_INSPECTOR
bool inspector_enabled_;
#endif // HAVE_INSPECTOR
bool wait_connect_;
bool http_enabled_;
bool deprecated_debug_;
bool break_first_line_;
std::string host_name_;
int port_;
};
Expand Down
4 changes: 2 additions & 2 deletions test/inspector/inspector-helper.js
Expand Up @@ -496,9 +496,9 @@ Harness.prototype.kill = function() {
};

exports.startNodeForInspectorTest = function(callback,
inspectorFlag = '--inspect-brk',
inspectorFlags = ['--inspect-brk'],
opt_script_contents) {
const args = [inspectorFlag];
const args = [].concat(inspectorFlags);
if (opt_script_contents) {
args.push('-e', opt_script_contents);
} else {
Expand Down
57 changes: 57 additions & 0 deletions test/inspector/test-inspector-debug-brk.js
@@ -0,0 +1,57 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();
const assert = require('assert');
const helper = require('./inspector-helper.js');

function setupExpectBreakOnLine(line, url, session, scopeIdCallback) {
return function(message) {
if ('Debugger.paused' === message['method']) {
const callFrame = message['params']['callFrames'][0];
const location = callFrame['location'];
assert.strictEqual(url, session.scriptUrlForId(location['scriptId']));
assert.strictEqual(line, location['lineNumber']);
scopeIdCallback &&
scopeIdCallback(callFrame['scopeChain'][0]['object']['objectId']);
return true;
}
};
}

function testBreakpointOnStart(session) {
const commands = [
{ 'method': 'Runtime.enable' },
{ 'method': 'Debugger.enable' },
{ 'method': 'Debugger.setPauseOnExceptions',
'params': {'state': 'none'} },
{ 'method': 'Debugger.setAsyncCallStackDepth',
'params': {'maxDepth': 0} },
{ 'method': 'Profiler.enable' },
{ 'method': 'Profiler.setSamplingInterval',
'params': {'interval': 100} },
{ 'method': 'Debugger.setBlackboxPatterns',
'params': {'patterns': []} },
{ 'method': 'Runtime.runIfWaitingForDebugger' }
];

session
.sendInspectorCommands(commands)
.expectMessages(setupExpectBreakOnLine(0, session.mainScriptPath, session));
}

function testWaitsForFrontendDisconnect(session, harness) {
console.log('[test]', 'Verify node waits for the frontend to disconnect');
session.sendInspectorCommands({ 'method': 'Debugger.resume'})
.expectStderrOutput('Waiting for the debugger to disconnect...')
.disconnect(true);
}

function runTests(harness) {
harness
.runFrontendSession([
testBreakpointOnStart,
testWaitsForFrontendDisconnect
]).expectShutDown(55);
}

helper.startNodeForInspectorTest(runTests, ['--inspect', '--debug-brk']);
23 changes: 23 additions & 0 deletions test/parallel/test-inspector-invalid-args.js
@@ -0,0 +1,23 @@
'use strict';
const assert = require('assert');
const execFile = require('child_process').execFile;
const path = require('path');

const common = require('../common');
common.skipIfInspectorDisabled();

const mainScript = path.join(common.fixturesDir, 'loop.js');
const expected =
'`node --debug` and `node --debug-brk` are invalid. ' +
'Please use `node --inspect` or `node --inspect-brk` instead.';
for (const invalidArg of ['--debug-brk', '--debug']) {
execFile(
process.execPath,
[ invalidArg, mainScript ],
common.mustCall((error, stdout, stderr) => {
assert.strictEqual(error.code, 9, `node ${invalidArg} should exit 9`);
assert.strictEqual(stderr.includes(expected), true,
`${stderr} should include '${expected}'`);
})
);
}
27 changes: 27 additions & 0 deletions test/sequential/test-debugger-debug-brk.js
@@ -0,0 +1,27 @@
'use strict';
const common = require('../common');
common.skipIfInspectorDisabled();
const assert = require('assert');
const spawn = require('child_process').spawn;

const script = common.fixturesDir + '/empty.js';

function test(arg) {
const child = spawn(process.execPath, ['--inspect', arg, script]);
const argStr = child.spawnargs.join(' ');
const fail = () => assert.fail(true, false, `'${argStr}' should not quit`);
child.on('exit', fail);

// give node time to start up the debugger
setTimeout(function() {
child.removeListener('exit', fail);
child.kill();
}, 2000);

process.on('exit', function() {
assert(child.killed);
});
}

test('--debug-brk');
test('--debug-brk=5959');

0 comments on commit 16689e3

Please sign in to comment.