Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 824dcfc

Browse filesBrowse files
joyeecheungRafaelGSS
joyeecheung
authored and
RafaelGSS
committed
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 <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent b2e6048 commit 824dcfc
Copy full SHA for 824dcfc

File tree

Expand file treeCollapse file tree

6 files changed

+63
-11
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+63
-11
lines changed
Open diff view settings
Collapse file

‎src/api/environment.cc‎

Copy file name to clipboardExpand all lines: src/api/environment.cc
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,6 @@ Environment* CreateEnvironment(
370370
Environment* env = new Environment(
371371
isolate_data, context, args, exec_args, nullptr, flags, thread_id);
372372
#if HAVE_INSPECTOR
373-
// TODO(joyeecheung): handle the exit code returned by InitializeInspector().
374373
if (env->should_create_inspector()) {
375374
if (inspector_parent_handle) {
376375
env->InitializeInspector(
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,7 @@ class Environment : public MemoryRetainer {
617617
#if HAVE_INSPECTOR
618618
// If the environment is created for a worker, pass parent_handle and
619619
// the ownership if transferred into the Environment.
620-
ExitCode InitializeInspector(
620+
void InitializeInspector(
621621
std::unique_ptr<inspector::ParentInspectorHandle> parent_handle);
622622
#endif
623623

Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ void SignalExit(int signo, siginfo_t* info, void* ucontext) {
166166
#endif // __POSIX__
167167

168168
#if HAVE_INSPECTOR
169-
ExitCode Environment::InitializeInspector(
169+
void Environment::InitializeInspector(
170170
std::unique_ptr<inspector::ParentInspectorHandle> parent_handle) {
171171
std::string inspector_path;
172172
bool is_main = !parent_handle;
@@ -187,7 +187,7 @@ ExitCode Environment::InitializeInspector(
187187
is_main);
188188
if (options_->debug_options().inspector_enabled &&
189189
!inspector_agent_->IsListening()) {
190-
return ExitCode::kInvalidCommandLineArgument2; // Signal internal error
190+
return;
191191
}
192192

193193
profiler::StartProfilers(this);
@@ -196,7 +196,7 @@ ExitCode Environment::InitializeInspector(
196196
inspector_agent_->PauseOnNextJavascriptStatement("Break at bootstrap");
197197
}
198198

199-
return ExitCode::kNoFailure;
199+
return;
200200
}
201201
#endif // HAVE_INSPECTOR
202202

Collapse file

‎src/node_main_instance.cc‎

Copy file name to clipboardExpand all lines: src/node_main_instance.cc
-4Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,6 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) {
181181
SetIsolateErrorHandlers(isolate_, {});
182182
env->InitializeMainContext(context, &(snapshot_data_->env_info));
183183
#if HAVE_INSPECTOR
184-
// TODO(joyeecheung): handle the exit code returned by
185-
// InitializeInspector().
186184
env->InitializeInspector({});
187185
#endif
188186

@@ -201,8 +199,6 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) {
201199
EnvironmentFlags::kDefaultFlags,
202200
{}));
203201
#if HAVE_INSPECTOR
204-
// TODO(joyeecheung): handle the exit code returned by
205-
// InitializeInspector().
206202
env->InitializeInspector({});
207203
#endif
208204
if (env->principal_realm()->RunBootstrapping().IsEmpty()) {
Collapse file

‎src/node_snapshotable.cc‎

Copy file name to clipboardExpand all lines: src/node_snapshotable.cc
-2Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,8 +1173,6 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out,
11731173
// in the future).
11741174
if (snapshot_type == SnapshotMetadata::Type::kFullyCustomized) {
11751175
#if HAVE_INSPECTOR
1176-
// TODO(joyeecheung): handle the exit code returned by
1177-
// InitializeInspector().
11781176
env->InitializeInspector({});
11791177
#endif
11801178
if (LoadEnvironment(env, StartExecutionCallback{}).IsEmpty()) {
Collapse file

‎test/parallel/test-inspect-address-in-use.js‎

Copy file name to clipboard
+59Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
'use strict';
2+
const common = require('../common');
3+
common.skipIfInspectorDisabled();
4+
5+
const { spawnSync } = require('child_process');
6+
const { createServer } = require('http');
7+
const assert = require('assert');
8+
const fixtures = require('../common/fixtures');
9+
const entry = fixtures.path('empty.js');
10+
const { Worker } = require('worker_threads');
11+
12+
function testOnServerListen(fn) {
13+
const server = createServer((socket) => {
14+
socket.end('echo');
15+
});
16+
17+
server.on('listening', () => {
18+
fn(server);
19+
server.close();
20+
});
21+
server.listen(0, '127.0.0.1');
22+
}
23+
24+
function testChildProcess(getArgs, exitCode) {
25+
testOnServerListen((server) => {
26+
const { port } = server.address();
27+
const child = spawnSync(process.execPath, getArgs(port));
28+
const stderr = child.stderr.toString().trim();
29+
const stdout = child.stdout.toString().trim();
30+
console.log('[STDERR]');
31+
console.log(stderr);
32+
console.log('[STDOUT]');
33+
console.log(stdout);
34+
const match = stderr.match(
35+
/Starting inspector on 127\.0\.0\.1:(\d+) failed: address already in use/
36+
);
37+
assert.notStrictEqual(match, null);
38+
assert.strictEqual(match[1], port + '');
39+
assert.strictEqual(child.status, exitCode);
40+
});
41+
}
42+
43+
testChildProcess(
44+
(port) => [`--inspect=${port}`, '--build-snapshot', entry], 0);
45+
testChildProcess(
46+
(port) => [`--inspect=${port}`, entry], 0);
47+
48+
testOnServerListen((server) => {
49+
const { port } = server.address();
50+
const worker = new Worker(entry, {
51+
execArgv: [`--inspect=${port}`]
52+
});
53+
54+
worker.on('error', common.mustNotCall());
55+
56+
worker.on('exit', common.mustCall((code) => {
57+
assert.strictEqual(code, 0);
58+
}));
59+
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.