From 1f2ffe8af30f2cac97f92bc88032a1ac3f888e24 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 14 Oct 2022 15:53:19 +0200 Subject: [PATCH] src: return void in InitializeInspector() We have been ignoring inspector port binding errors during startup. Handling this error would be a breaking change and it's probably surprising to refuse to launch the Node.js instance simply because the inspector cannot listen to the port anyway. So just turn the return value of InitializeInspector() and remove the TODOs for handling the error. PR-URL: https://github.com/nodejs/node/pull/44903 Reviewed-By: Chengzhong Wu Reviewed-By: James M Snell --- src/api/environment.cc | 1 - src/env.h | 2 +- src/node.cc | 6 +- src/node_main_instance.cc | 4 -- src/node_snapshotable.cc | 2 - test/parallel/test-inspect-address-in-use.js | 59 ++++++++++++++++++++ 6 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 test/parallel/test-inspect-address-in-use.js diff --git a/src/api/environment.cc b/src/api/environment.cc index 635c96c17a2be4..6bc4033b97da5a 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -370,7 +370,6 @@ Environment* CreateEnvironment( Environment* env = new Environment( isolate_data, context, args, exec_args, nullptr, flags, thread_id); #if HAVE_INSPECTOR - // TODO(joyeecheung): handle the exit code returned by InitializeInspector(). if (env->should_create_inspector()) { if (inspector_parent_handle) { env->InitializeInspector( diff --git a/src/env.h b/src/env.h index 9e72e5541c3a9b..a5b15863574a56 100644 --- a/src/env.h +++ b/src/env.h @@ -617,7 +617,7 @@ class Environment : public MemoryRetainer { #if HAVE_INSPECTOR // If the environment is created for a worker, pass parent_handle and // the ownership if transferred into the Environment. - ExitCode InitializeInspector( + void InitializeInspector( std::unique_ptr parent_handle); #endif diff --git a/src/node.cc b/src/node.cc index a9b909b74e0e6b..a6a4cb79250db6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -166,7 +166,7 @@ void SignalExit(int signo, siginfo_t* info, void* ucontext) { #endif // __POSIX__ #if HAVE_INSPECTOR -ExitCode Environment::InitializeInspector( +void Environment::InitializeInspector( std::unique_ptr parent_handle) { std::string inspector_path; bool is_main = !parent_handle; @@ -187,7 +187,7 @@ ExitCode Environment::InitializeInspector( is_main); if (options_->debug_options().inspector_enabled && !inspector_agent_->IsListening()) { - return ExitCode::kInvalidCommandLineArgument2; // Signal internal error + return; } profiler::StartProfilers(this); @@ -196,7 +196,7 @@ ExitCode Environment::InitializeInspector( inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap"); } - return ExitCode::kNoFailure; + return; } #endif // HAVE_INSPECTOR diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 6ded80aa05b878..3e0167fdddea12 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -181,8 +181,6 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) { SetIsolateErrorHandlers(isolate_, {}); env->InitializeMainContext(context, &(snapshot_data_->env_info)); #if HAVE_INSPECTOR - // TODO(joyeecheung): handle the exit code returned by - // InitializeInspector(). env->InitializeInspector({}); #endif @@ -201,8 +199,6 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) { EnvironmentFlags::kDefaultFlags, {})); #if HAVE_INSPECTOR - // TODO(joyeecheung): handle the exit code returned by - // InitializeInspector(). env->InitializeInspector({}); #endif if (env->principal_realm()->RunBootstrapping().IsEmpty()) { diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index 6a6b899cb83529..a5fda9aa5a3833 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1173,8 +1173,6 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, // in the future). if (snapshot_type == SnapshotMetadata::Type::kFullyCustomized) { #if HAVE_INSPECTOR - // TODO(joyeecheung): handle the exit code returned by - // InitializeInspector(). env->InitializeInspector({}); #endif if (LoadEnvironment(env, StartExecutionCallback{}).IsEmpty()) { diff --git a/test/parallel/test-inspect-address-in-use.js b/test/parallel/test-inspect-address-in-use.js new file mode 100644 index 00000000000000..a5b2d98537dfe4 --- /dev/null +++ b/test/parallel/test-inspect-address-in-use.js @@ -0,0 +1,59 @@ +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); + +const { spawnSync } = require('child_process'); +const { createServer } = require('http'); +const assert = require('assert'); +const fixtures = require('../common/fixtures'); +const entry = fixtures.path('empty.js'); +const { Worker } = require('worker_threads'); + +function testOnServerListen(fn) { + const server = createServer((socket) => { + socket.end('echo'); + }); + + server.on('listening', () => { + fn(server); + server.close(); + }); + server.listen(0, '127.0.0.1'); +} + +function testChildProcess(getArgs, exitCode) { + testOnServerListen((server) => { + const { port } = server.address(); + const child = spawnSync(process.execPath, getArgs(port)); + const stderr = child.stderr.toString().trim(); + const stdout = child.stdout.toString().trim(); + console.log('[STDERR]'); + console.log(stderr); + console.log('[STDOUT]'); + console.log(stdout); + const match = stderr.match( + /Starting inspector on 127\.0\.0\.1:(\d+) failed: address already in use/ + ); + assert.notStrictEqual(match, null); + assert.strictEqual(match[1], port + ''); + assert.strictEqual(child.status, exitCode); + }); +} + +testChildProcess( + (port) => [`--inspect=${port}`, '--build-snapshot', entry], 0); +testChildProcess( + (port) => [`--inspect=${port}`, entry], 0); + +testOnServerListen((server) => { + const { port } = server.address(); + const worker = new Worker(entry, { + execArgv: [`--inspect=${port}`] + }); + + worker.on('error', common.mustNotCall()); + + worker.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); +});