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: #44903 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Nov 10, 2022 · 824dcfc · 824dcfc
1 parent b2e6048
commit 824dcfc
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 11 deletions.
diff --git 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
@@ -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<inspector::ParentInspectorHandle> parent_handle);
 #endif
 

diff --git 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<inspector::ParentInspectorHandle> 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
@@ -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
@@ -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
@@ -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);
+  }));
+});