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 56b2a72

Browse filesBrowse files
joyeecheungBethGriggs
joyeecheung
authored and
BethGriggs
committed
inspector: split the HostPort being used and the one parsed from CLI
Instead of using a shared pointer of the entire debug option set, pass the parsed debug option to inspector classes by value because they are set once the CLI argument parsing is done. Add another shared pointer to HostPort being used by the inspector server, which is copied from the one in the debug options initially. The port of the shared HostPort is 9229 by default and can be specified as 0 initially but will be set to the actual port of the server once it starts listening. This makes the shared state clearer and makes it possible to use `require('internal/options')` in JS land to query the CLI options instead of using `process._breakFirstLine` and other underscored properties of `process` since we are now certain that these values should not be altered once the parsing is done and can be passed around in copies without locks. PR-URL: #24772 Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent 81dce68 commit 56b2a72
Copy full SHA for 56b2a72
Expand file treeCollapse file tree

15 files changed

+160
-98
lines changed
Open diff view settings
Collapse file

‎src/env-inl.h‎

Copy file name to clipboardExpand all lines: src/env-inl.h
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,10 @@ inline std::shared_ptr<EnvironmentOptions> Environment::options() {
585585
return options_;
586586
}
587587

588+
inline std::shared_ptr<HostPort> Environment::inspector_host_port() {
589+
return inspector_host_port_;
590+
}
591+
588592
inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
589593
return options_;
590594
}
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include "node_file.h"
66
#include "node_internals.h"
77
#include "node_native_module.h"
8+
#include "node_options-inl.h"
89
#include "node_platform.h"
910
#include "node_worker.h"
1011
#include "tracing/agent.h"
@@ -192,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data,
192193
// part of the per-Isolate option set, for which in turn the defaults are
193194
// part of the per-process option set.
194195
options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env));
195-
options_->debug_options.reset(new DebugOptions(*options_->debug_options));
196+
inspector_host_port_.reset(new HostPort(options_->debug_options().host_port));
196197

197198
#if HAVE_INSPECTOR
198199
// We can only create the inspector agent after having cloned the options.
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,7 @@ class Environment {
911911
void* data);
912912

913913
inline std::shared_ptr<EnvironmentOptions> options();
914+
inline std::shared_ptr<HostPort> inspector_host_port();
914915

915916
private:
916917
inline void CreateImmediate(native_immediate_callback cb,
@@ -942,6 +943,14 @@ class Environment {
942943
std::vector<double> destroy_async_id_list_;
943944

944945
std::shared_ptr<EnvironmentOptions> options_;
946+
// options_ contains debug options parsed from CLI arguments,
947+
// while inspector_host_port_ stores the actual inspector host
948+
// and port being used. For example the port is -1 by default
949+
// and can be specified as 0 (meaning any port allocated when the
950+
// server starts listening), but when the inspector server starts
951+
// the inspector_host_port_->port() will be the actual port being
952+
// used.
953+
std::shared_ptr<HostPort> inspector_host_port_;
945954

946955
uint32_t module_id_counter_ = 0;
947956
uint32_t script_id_counter_ = 0;
Collapse file

‎src/inspector_agent.cc‎

Copy file name to clipboardExpand all lines: src/inspector_agent.cc
+17-12Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -669,8 +669,9 @@ class NodeInspectorClient : public V8InspectorClient {
669669
};
670670

671671
Agent::Agent(Environment* env)
672-
: parent_env_(env),
673-
debug_options_(env->options()->debug_options) {}
672+
: parent_env_(env),
673+
debug_options_(env->options()->debug_options()),
674+
host_port_(env->inspector_host_port()) {}
674675

675676
Agent::~Agent() {
676677
if (start_io_thread_async.data == this) {
@@ -681,13 +682,14 @@ Agent::~Agent() {
681682
}
682683

683684
bool Agent::Start(const std::string& path,
684-
std::shared_ptr<DebugOptions> options,
685+
const DebugOptions& options,
686+
std::shared_ptr<HostPort> host_port,
685687
bool is_main) {
686-
if (options == nullptr) {
687-
options = std::make_shared<DebugOptions>();
688-
}
689688
path_ = path;
690689
debug_options_ = options;
690+
CHECK_NE(host_port, nullptr);
691+
host_port_ = host_port;
692+
691693
client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
692694
if (parent_env_->is_main_thread()) {
693695
CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
@@ -699,13 +701,18 @@ bool Agent::Start(const std::string& path,
699701
StartDebugSignalHandler();
700702
}
701703

702-
bool wait_for_connect = options->wait_for_connect();
704+
bool wait_for_connect = options.wait_for_connect();
703705
if (parent_handle_) {
704706
wait_for_connect = parent_handle_->WaitForConnect();
705707
parent_handle_->WorkerStarted(client_->getThreadHandle(), wait_for_connect);
706-
} else if (!options->inspector_enabled || !StartIoThread()) {
708+
} else if (!options.inspector_enabled || !StartIoThread()) {
707709
return false;
708710
}
711+
712+
// TODO(joyeecheung): we should not be using process as a global object
713+
// to transport --inspect-brk. Instead, the JS land can get this through
714+
// require('internal/options') since it should be set once CLI parsing
715+
// is done.
709716
if (wait_for_connect) {
710717
HandleScope scope(parent_env_->isolate());
711718
parent_env_->process_object()->DefineOwnProperty(
@@ -725,8 +732,7 @@ bool Agent::StartIoThread() {
725732

726733
CHECK_NOT_NULL(client_);
727734

728-
io_ = InspectorIo::Start(
729-
client_->getThreadHandle(), path_, debug_options_);
735+
io_ = InspectorIo::Start(client_->getThreadHandle(), path_, host_port_);
730736
if (io_ == nullptr) {
731737
return false;
732738
}
@@ -867,8 +873,7 @@ void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {
867873
}
868874

869875
bool Agent::WillWaitForConnect() {
870-
if (debug_options_->wait_for_connect())
871-
return true;
876+
if (debug_options_.wait_for_connect()) return true;
872877
if (parent_handle_)
873878
return parent_handle_->WaitForConnect();
874879
return false;
Collapse file

‎src/inspector_agent.h‎

Copy file name to clipboardExpand all lines: src/inspector_agent.h
+13-5Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#error("This header can only be used when inspector is enabled")
1212
#endif
1313

14-
#include "node_options.h"
14+
#include "node_options-inl.h"
1515
#include "node_persistent.h"
1616
#include "v8.h"
1717

@@ -50,8 +50,9 @@ class Agent {
5050

5151
// Create client_, may create io_ if option enabled
5252
bool Start(const std::string& path,
53-
std::shared_ptr<DebugOptions> options,
54-
bool is_worker);
53+
const DebugOptions& options,
54+
std::shared_ptr<HostPort> host_port,
55+
bool is_main);
5556
// Stop and destroy io_
5657
void Stop();
5758

@@ -104,7 +105,8 @@ class Agent {
104105
// Calls StartIoThread() from off the main thread.
105106
void RequestIoThreadStart();
106107

107-
std::shared_ptr<DebugOptions> options() { return debug_options_; }
108+
const DebugOptions& options() { return debug_options_; }
109+
std::shared_ptr<HostPort> host_port() { return host_port_; }
108110
void ContextCreated(v8::Local<v8::Context> context, const ContextInfo& info);
109111

110112
// Interface for interacting with inspectors in worker threads
@@ -121,7 +123,13 @@ class Agent {
121123
std::unique_ptr<InspectorIo> io_;
122124
std::unique_ptr<ParentInspectorHandle> parent_handle_;
123125
std::string path_;
124-
std::shared_ptr<DebugOptions> debug_options_;
126+
127+
// This is a copy of the debug options parsed from CLI in the Environment.
128+
// Do not use the host_port in that, instead manipulate the shared host_port_
129+
// pointer which is meant to store the actual host and port of the inspector
130+
// server.
131+
DebugOptions debug_options_;
132+
std::shared_ptr<HostPort> host_port_;
125133

126134
bool pending_enable_async_hook_ = false;
127135
bool pending_disable_async_hook_ = false;
Collapse file

‎src/inspector_io.cc‎

Copy file name to clipboardExpand all lines: src/inspector_io.cc
+13-9Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,9 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
242242
std::unique_ptr<InspectorIo> InspectorIo::Start(
243243
std::shared_ptr<MainThreadHandle> main_thread,
244244
const std::string& path,
245-
std::shared_ptr<DebugOptions> options) {
245+
std::shared_ptr<HostPort> host_port) {
246246
auto io = std::unique_ptr<InspectorIo>(
247-
new InspectorIo(main_thread, path, options));
247+
new InspectorIo(main_thread, path, host_port));
248248
if (io->request_queue_->Expired()) { // Thread is not running
249249
return nullptr;
250250
}
@@ -253,9 +253,12 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(
253253

254254
InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
255255
const std::string& path,
256-
std::shared_ptr<DebugOptions> options)
257-
: main_thread_(main_thread), options_(options),
258-
thread_(), script_name_(path), id_(GenerateID()) {
256+
std::shared_ptr<HostPort> host_port)
257+
: main_thread_(main_thread),
258+
host_port_(host_port),
259+
thread_(),
260+
script_name_(path),
261+
id_(GenerateID()) {
259262
Mutex::ScopedLock scoped_lock(thread_start_lock_);
260263
CHECK_EQ(uv_thread_create(&thread_, InspectorIo::ThreadMain, this), 0);
261264
thread_start_condition_.Wait(scoped_lock);
@@ -287,16 +290,17 @@ void InspectorIo::ThreadMain() {
287290
std::unique_ptr<InspectorIoDelegate> delegate(
288291
new InspectorIoDelegate(queue, main_thread_, id_,
289292
script_path, script_name_));
290-
InspectorSocketServer server(std::move(delegate), &loop,
291-
options_->host().c_str(),
292-
options_->port());
293+
InspectorSocketServer server(std::move(delegate),
294+
&loop,
295+
host_port_->host().c_str(),
296+
host_port_->port());
293297
request_queue_ = queue->handle();
294298
// Its lifetime is now that of the server delegate
295299
queue.reset();
296300
{
297301
Mutex::ScopedLock scoped_lock(thread_start_lock_);
298302
if (server.Start()) {
299-
port_ = server.Port();
303+
host_port_->set_port(server.Port());
300304
}
301305
thread_start_condition_.Broadcast(scoped_lock);
302306
}
Collapse file

‎src/inspector_io.h‎

Copy file name to clipboardExpand all lines: src/inspector_io.h
+7-7Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,21 +46,22 @@ class InspectorIo {
4646
// bool Start();
4747
// Returns empty pointer if thread was not started
4848
static std::unique_ptr<InspectorIo> Start(
49-
std::shared_ptr<MainThreadHandle> main_thread, const std::string& path,
50-
std::shared_ptr<DebugOptions> options);
49+
std::shared_ptr<MainThreadHandle> main_thread,
50+
const std::string& path,
51+
std::shared_ptr<HostPort> host_port);
5152

5253
// Will block till the transport thread shuts down
5354
~InspectorIo();
5455

5556
void StopAcceptingNewConnections();
56-
const std::string& host() const { return options_->host(); }
57-
int port() const { return port_; }
57+
const std::string& host() const { return host_port_->host(); }
58+
int port() const { return host_port_->port(); }
5859
std::vector<std::string> GetTargetIds() const;
5960

6061
private:
6162
InspectorIo(std::shared_ptr<MainThreadHandle> handle,
6263
const std::string& path,
63-
std::shared_ptr<DebugOptions> options);
64+
std::shared_ptr<HostPort> host_port);
6465

6566
// Wrapper for agent->ThreadMain()
6667
static void ThreadMain(void* agent);
@@ -74,7 +75,7 @@ class InspectorIo {
7475
// Used to post on a frontend interface thread, lives while the server is
7576
// running
7677
std::shared_ptr<RequestQueue> request_queue_;
77-
std::shared_ptr<DebugOptions> options_;
78+
std::shared_ptr<HostPort> host_port_;
7879

7980
// The IO thread runs its own uv_loop to implement the TCP server off
8081
// the main thread.
@@ -84,7 +85,6 @@ class InspectorIo {
8485
Mutex thread_start_lock_;
8586
ConditionVariable thread_start_condition_;
8687
std::string script_name_;
87-
int port_ = -1;
8888
// May be accessed from any thread
8989
const std::string id_;
9090
};
Collapse file

‎src/inspector_js_api.cc‎

Copy file name to clipboardExpand all lines: src/inspector_js_api.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,12 +243,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {
243243

244244
if (args.Length() > 0 && args[0]->IsUint32()) {
245245
uint32_t port = args[0].As<Uint32>()->Value();
246-
agent->options()->host_port.port = port;
246+
agent->host_port()->set_port(static_cast<int>(port));
247247
}
248248

249249
if (args.Length() > 1 && args[1]->IsString()) {
250250
Utf8Value host(env->isolate(), args[1].As<String>());
251-
agent->options()->host_port.host_name = *host;
251+
agent->host_port()->set_host(*host);
252252
}
253253

254254
if (args.Length() > 2 && args[2]->IsBoolean()) {
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+16-16Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include "node_internals.h"
2828
#include "node_metadata.h"
2929
#include "node_native_module.h"
30+
#include "node_options-inl.h"
3031
#include "node_perf.h"
3132
#include "node_platform.h"
3233
#include "node_revert.h"
@@ -259,13 +260,15 @@ static struct {
259260
}
260261

261262
#if HAVE_INSPECTOR
262-
bool StartInspector(Environment* env, const char* script_path,
263-
std::shared_ptr<DebugOptions> options) {
263+
bool StartInspector(Environment* env, const char* script_path) {
264264
// Inspector agent can't fail to start, but if it was configured to listen
265265
// right away on the websocket port and fails to bind/etc, this will return
266266
// false.
267267
return env->inspector_agent()->Start(
268-
script_path == nullptr ? "" : script_path, options, true);
268+
script_path == nullptr ? "" : script_path,
269+
env->options()->debug_options(),
270+
env->inspector_host_port(),
271+
true);
269272
}
270273

271274
bool InspectorStarted(Environment* env) {
@@ -306,8 +309,7 @@ static struct {
306309
void Dispose() {}
307310
void DrainVMTasks(Isolate* isolate) {}
308311
void CancelVMTasks(Isolate* isolate) {}
309-
bool StartInspector(Environment* env, const char* script_path,
310-
const DebugOptions& options) {
312+
bool StartInspector(Environment* env, const char* script_path) {
311313
env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
312314
return true;
313315
}
@@ -1127,24 +1129,24 @@ void SetupProcessObject(Environment* env,
11271129

11281130
// TODO(refack): move the following 4 to `node_config`
11291131
// --inspect-brk
1130-
if (env->options()->debug_options->wait_for_connect()) {
1132+
if (env->options()->debug_options().wait_for_connect()) {
11311133
READONLY_DONT_ENUM_PROPERTY(process,
11321134
"_breakFirstLine", True(env->isolate()));
11331135
}
11341136

1135-
if (env->options()->debug_options->break_node_first_line) {
1137+
if (env->options()->debug_options().break_node_first_line) {
11361138
READONLY_DONT_ENUM_PROPERTY(process,
11371139
"_breakNodeFirstLine", True(env->isolate()));
11381140
}
11391141

11401142
// --inspect --debug-brk
1141-
if (env->options()->debug_options->deprecated_invocation()) {
1143+
if (env->options()->debug_options().deprecated_invocation()) {
11421144
READONLY_DONT_ENUM_PROPERTY(process,
11431145
"_deprecatedDebugBrk", True(env->isolate()));
11441146
}
11451147

11461148
// --debug or, --debug-brk without --inspect
1147-
if (env->options()->debug_options->invalid_invocation()) {
1149+
if (env->options()->debug_options().invalid_invocation()) {
11481150
READONLY_DONT_ENUM_PROPERTY(process,
11491151
"_invalidDebug", True(env->isolate()));
11501152
}
@@ -1356,7 +1358,7 @@ void LoadEnvironment(Environment* env) {
13561358
get_linked_binding_fn,
13571359
get_internal_binding_fn,
13581360
Boolean::New(env->isolate(),
1359-
env->options()->debug_options->break_node_first_line)
1361+
env->options()->debug_options().break_node_first_line)
13601362
};
13611363

13621364
// Bootstrap internal loaders
@@ -1390,12 +1392,10 @@ void LoadEnvironment(Environment* env) {
13901392
}
13911393
}
13921394

1393-
1394-
static void StartInspector(Environment* env, const char* path,
1395-
std::shared_ptr<DebugOptions> debug_options) {
1395+
static void StartInspector(Environment* env, const char* path) {
13961396
#if HAVE_INSPECTOR
13971397
CHECK(!env->inspector_agent()->IsListening());
1398-
v8_platform.StartInspector(env, path, debug_options);
1398+
v8_platform.StartInspector(env, path);
13991399
#endif // HAVE_INSPECTOR
14001400
}
14011401

@@ -2038,9 +2038,9 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
20382038
env.Start(args, exec_args, v8_is_profiling);
20392039

20402040
const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
2041-
StartInspector(&env, path, env.options()->debug_options);
2041+
StartInspector(&env, path);
20422042

2043-
if (env.options()->debug_options->inspector_enabled &&
2043+
if (env.options()->debug_options().inspector_enabled &&
20442044
!v8_platform.InspectorStarted(&env)) {
20452045
return 12; // Signal internal error.
20462046
}

0 commit comments

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