From 29123002964e56b525a7bc88a0ae3c898b83a99d Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 23 Apr 2020 12:58:24 +0800 Subject: [PATCH 1/2] inspector: throw error when activating an already active inspector When the user tries to activate the inspector that is already active on a different port and host, we previously just silently reset the port and host stored in the Environment without actually doing anything for that to be effective. After this patch, we throw an error telling the user to close the active inspector before invoking `inspector.open()` again. --- doc/api/errors.md | 7 ++++++ lib/inspector.js | 5 +++++ lib/internal/errors.js | 4 ++++ .../test-inspector-already-activated-cli.js | 18 +++++++++++++++ test/sequential/test-inspector-open.js | 22 +++++++++++++++---- 5 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 test/sequential/test-inspector-already-activated-cli.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 8f3af6fcb6a9d6..b1954138847c2f 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1210,6 +1210,13 @@ time. The `--input-type` flag was used to attempt to execute a file. This flag can only be used with input via `--eval`, `--print` or `STDIN`. + +### `ERR_INSPECTOR_ALREADY_ACTIVATED` + +While using the `inspector` module, an attempt was made to activate the +inspector when it already started to listen on a port. Use `inspector.close()` +before activating it on a different address. + ### `ERR_INSPECTOR_ALREADY_CONNECTED` diff --git a/lib/inspector.js b/lib/inspector.js index 269f95dbd27a12..60640bc1fb6456 100644 --- a/lib/inspector.js +++ b/lib/inspector.js @@ -8,6 +8,7 @@ const { } = primordials; const { + ERR_INSPECTOR_ALREADY_ACTIVATED, ERR_INSPECTOR_ALREADY_CONNECTED, ERR_INSPECTOR_CLOSED, ERR_INSPECTOR_COMMAND, @@ -33,6 +34,7 @@ const { MainThreadConnection, open, url, + isEnabled, waitForDebugger } = internalBinding('inspector'); @@ -131,6 +133,9 @@ class Session extends EventEmitter { } function inspectorOpen(port, host, wait) { + if (isEnabled()) { + throw new ERR_INSPECTOR_ALREADY_ACTIVATED(); + } open(port, host); if (wait) waitForDebugger(); diff --git a/lib/internal/errors.js b/lib/internal/errors.js index e0668e2f827e5f..dfb64571ef62f7 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -920,6 +920,10 @@ E('ERR_INCOMPATIBLE_OPTION_PAIR', 'Option "%s" cannot be used in combination with option "%s"', TypeError); E('ERR_INPUT_TYPE_NOT_ALLOWED', '--input-type can only be used with string ' + 'input via --eval, --print, or STDIN', Error); +E('ERR_INSPECTOR_ALREADY_ACTIVATED', + 'Inspector is already activated. Close it with inspector.close() ' + + 'before activating it again.', + Error); E('ERR_INSPECTOR_ALREADY_CONNECTED', '%s is already connected', Error); E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error); E('ERR_INSPECTOR_COMMAND', 'Inspector error %d: %s', Error); diff --git a/test/sequential/test-inspector-already-activated-cli.js b/test/sequential/test-inspector-already-activated-cli.js new file mode 100644 index 00000000000000..3b959948a1585b --- /dev/null +++ b/test/sequential/test-inspector-already-activated-cli.js @@ -0,0 +1,18 @@ +// Flags: --inspect=0 +'use strict'; + +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const inspector = require('inspector'); +const wsUrl = inspector.url(); +assert(wsUrl.startsWith('ws://')); +assert.throws(() => { + inspector.open(0, undefined, false); +}, { + code: 'ERR_INSPECTOR_ALREADY_ACTIVATED' +}) +assert.strictEqual(inspector.url(), wsUrl); +inspector.close(); +assert.strictEqual(inspector.url(), undefined); diff --git a/test/sequential/test-inspector-open.js b/test/sequential/test-inspector-open.js index 967ebe49bfb764..190a99e7282e52 100644 --- a/test/sequential/test-inspector-open.js +++ b/test/sequential/test-inspector-open.js @@ -10,6 +10,10 @@ const fork = require('child_process').fork; const net = require('net'); const url = require('url'); +const kFirstOpen = 0; +const kOpenWhileOpen = 1; +const kReOpen = 2; + if (process.env.BE_CHILD) return beChild(); @@ -19,7 +23,7 @@ const child = fork(__filename, child.once('message', common.mustCall((msg) => { assert.strictEqual(msg.cmd, 'started'); - child.send({ cmd: 'open', args: [0] }); + child.send({ cmd: 'open', args: [kFirstOpen] }); child.once('message', common.mustCall(firstOpen)); })); @@ -31,7 +35,7 @@ function firstOpen(msg) { ping(port, (err) => { assert.ifError(err); // Inspector is already open, and won't be reopened, so args don't matter. - child.send({ cmd: 'open', args: [] }); + child.send({ cmd: 'open', args: [kOpenWhileOpen] }); child.once('message', common.mustCall(tryToOpenWhenOpen)); firstPort = port; }); @@ -62,7 +66,7 @@ function closeWhenOpen(msg) { function tryToCloseWhenClosed(msg) { assert.strictEqual(msg.cmd, 'url'); assert.strictEqual(msg.url, undefined); - child.send({ cmd: 'open', args: [] }); + child.send({ cmd: 'open', args: [kReOpen] }); child.once('message', common.mustCall(reopenAfterClose)); } @@ -93,7 +97,17 @@ function beChild() { process.on('message', (msg) => { if (msg.cmd === 'open') { - inspector.open(...msg.args); + if (msg.args[0] === kFirstOpen) { + inspector.open(0, false, undefined); + } else if (msg.args[0] === kOpenWhileOpen) { + assert.throws(() => { + inspector.open(0, false, undefined); + }, { + code: 'ERR_INSPECTOR_ALREADY_ACTIVATED' + }); + } else if (msg.args[0] === kReOpen) { + inspector.open(0, false, undefined); + } } if (msg.cmd === 'close') { inspector.close(); From 08d30d975006073bd84149ac9ecdf7fec36774a0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 30 Apr 2020 10:20:46 +0800 Subject: [PATCH 2/2] fixup! inspector: throw error when activating an already active inspector --- test/sequential/test-inspector-already-activated-cli.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/sequential/test-inspector-already-activated-cli.js b/test/sequential/test-inspector-already-activated-cli.js index 3b959948a1585b..ba76d5168c14b9 100644 --- a/test/sequential/test-inspector-already-activated-cli.js +++ b/test/sequential/test-inspector-already-activated-cli.js @@ -3,6 +3,7 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +common.skipIfWorker(); const assert = require('assert'); const inspector = require('inspector'); @@ -12,7 +13,7 @@ assert.throws(() => { inspector.open(0, undefined, false); }, { code: 'ERR_INSPECTOR_ALREADY_ACTIVATED' -}) +}); assert.strictEqual(inspector.url(), wsUrl); inspector.close(); assert.strictEqual(inspector.url(), undefined);