Skip to content

Commit 29a71ba

Browse files
addaleaxaddaleax
committed
src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: nodejs#22392 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
1 parent 92880f3 commit 29a71ba

24 files changed

+1354
-889
lines changed

‎node.gyp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,14 +343,14 @@
343343
'src/node_config.cc',
344344
'src/node_constants.cc',
345345
'src/node_contextify.cc',
346-
'src/node_debug_options.cc',
347346
'src/node_domain.cc',
348347
'src/node_encoding.cc',
349348
'src/node_errors.h',
350349
'src/node_file.cc',
351350
'src/node_http2.cc',
352351
'src/node_http_parser.cc',
353352
'src/node_messaging.cc',
353+
'src/node_options.cc',
354354
'src/node_os.cc',
355355
'src/node_platform.cc',
356356
'src/node_perf.cc',
@@ -407,14 +407,15 @@
407407
'src/node_code_cache.h',
408408
'src/node_constants.h',
409409
'src/node_contextify.h',
410-
'src/node_debug_options.h',
411410
'src/node_file.h',
412411
'src/node_http2.h',
413412
'src/node_http2_state.h',
414413
'src/node_internals.h',
415414
'src/node_javascript.h',
416415
'src/node_messaging.h',
417416
'src/node_mutex.h',
417+
'src/node_options.h',
418+
'src/node_options-inl.h',
418419
'src/node_perf.h',
419420
'src/node_perf_common.h',
420421
'src/node_persistent.h',

‎src/env-inl.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -564,6 +564,14 @@ Environment::file_handle_read_wrap_freelist() {
564564
return file_handle_read_wrap_freelist_;
565565
}
566566

567+
inline std::shared_ptr<EnvironmentOptions> Environment::options() {
568+
return options_;
569+
}
570+
571+
inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
572+
return options_;
573+
}
574+
567575
void Environment::CreateImmediate(native_immediate_callback cb,
568576
void* data,
569577
v8::Local<v8::Object> obj,

‎src/env.cc‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ IsolateData::IsolateData(Isolate* isolate,
4848
if (platform_ != nullptr)
4949
platform_->RegisterIsolate(this, event_loop);
5050

51+
options_.reset(new PerIsolateOptions(*per_process_opts->per_isolate));
52+
5153
// Create string and private symbol properties as internalized one byte
5254
// strings after the platform is properly initialized.
5355
//
@@ -136,9 +138,6 @@ Environment::Environment(IsolateData* isolate_data,
136138
makecallback_cntr_(0),
137139
should_abort_on_uncaught_toggle_(isolate_, 1),
138140
trace_category_state_(isolate_, kTraceCategoryCount),
139-
#if HAVE_INSPECTOR
140-
inspector_agent_(new inspector::Agent(this)),
141-
#endif
142141
http_parser_buffer_(nullptr),
143142
fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2),
144143
fs_stats_field_bigint_array_(isolate_, kFsStatsFieldsLength * 2),
@@ -148,6 +147,19 @@ Environment::Environment(IsolateData* isolate_data,
148147
v8::Context::Scope context_scope(context);
149148
set_as_external(v8::External::New(isolate(), this));
150149

150+
// We create new copies of the per-Environment option sets, so that it is
151+
// easier to modify them after Environment creation. The defaults are
152+
// part of the per-Isolate option set, for which in turn the defaults are
153+
// part of the per-process option set.
154+
options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env));
155+
options_->debug_options.reset(new DebugOptions(*options_->debug_options));
156+
157+
#if HAVE_INSPECTOR
158+
// We can only create the inspector agent after having cloned the options.
159+
inspector_agent_ =
160+
std::unique_ptr<inspector::Agent>(new inspector::Agent(this));
161+
#endif
162+
151163
AssignToContext(context, ContextInfo(""));
152164

153165
if (tracing_agent_writer_ != nullptr) {
@@ -211,10 +223,8 @@ Environment::~Environment() {
211223
delete[] http_parser_buffer_;
212224
}
213225

214-
void Environment::Start(int argc,
215-
const char* const* argv,
216-
int exec_argc,
217-
const char* const* exec_argv,
226+
void Environment::Start(const std::vector<std::string>& args,
227+
const std::vector<std::string>& exec_args,
218228
bool start_profiler_idle_notifier) {
219229
HandleScope handle_scope(isolate());
220230
Context::Scope context_scope(context());
@@ -260,7 +270,7 @@ void Environment::Start(int argc,
260270
process_template->GetFunction()->NewInstance(context()).ToLocalChecked();
261271
set_process_object(process_object);
262272

263-
SetupProcessObject(this, argc, argv, exec_argc, exec_argv);
273+
SetupProcessObject(this, args, exec_args);
264274

265275
static uv_once_t init_once = UV_ONCE_INIT;
266276
uv_once(&init_once, InitThreadLocalOnce);

‎src/env.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include "uv.h"
3535
#include "v8.h"
3636
#include "node.h"
37+
#include "node_options.h"
3738
#include "node_http2_state.h"
3839

3940
#include <list>
@@ -365,6 +366,7 @@ class IsolateData {
365366
inline uv_loop_t* event_loop() const;
366367
inline uint32_t* zero_fill_field() const;
367368
inline MultiIsolatePlatform* platform() const;
369+
inline std::shared_ptr<PerIsolateOptions> options();
368370

369371
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
370372
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
@@ -398,6 +400,7 @@ class IsolateData {
398400
uv_loop_t* const event_loop_;
399401
uint32_t* const zero_fill_field_;
400402
MultiIsolatePlatform* platform_;
403+
std::shared_ptr<PerIsolateOptions> options_;
401404

402405
DISALLOW_COPY_AND_ASSIGN(IsolateData);
403406
};
@@ -582,10 +585,8 @@ class Environment {
582585
tracing::AgentWriterHandle* tracing_agent_writer);
583586
~Environment();
584587

585-
void Start(int argc,
586-
const char* const* argv,
587-
int exec_argc,
588-
const char* const* exec_argv,
588+
void Start(const std::vector<std::string>& args,
589+
const std::vector<std::string>& exec_args,
589590
bool start_profiler_idle_notifier);
590591

591592
typedef void (*HandleCleanupCb)(Environment* env,
@@ -882,6 +883,8 @@ class Environment {
882883
v8::EmbedderGraph* graph,
883884
void* data);
884885

886+
inline std::shared_ptr<EnvironmentOptions> options();
887+
885888
private:
886889
inline void CreateImmediate(native_immediate_callback cb,
887890
void* data,
@@ -912,6 +915,8 @@ class Environment {
912915
size_t makecallback_cntr_;
913916
std::vector<double> destroy_async_id_list_;
914917

918+
std::shared_ptr<EnvironmentOptions> options_;
919+
915920
AliasedBuffer<uint32_t, v8::Uint32Array> should_abort_on_uncaught_toggle_;
916921
int should_not_abort_scope_counter_ = 0;
917922

‎src/inspector_agent.cc‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -608,11 +608,14 @@ class NodeInspectorClient : public V8InspectorClient {
608608
std::unique_ptr<MainThreadInterface> interface_;
609609
};
610610

611-
Agent::Agent(Environment* env) : parent_env_(env) {}
611+
Agent::Agent(Environment* env)
612+
: parent_env_(env),
613+
debug_options_(env->options()->debug_options) {}
612614

613615
Agent::~Agent() = default;
614616

615-
bool Agent::Start(const std::string& path, const DebugOptions& options) {
617+
bool Agent::Start(const std::string& path,
618+
std::shared_ptr<DebugOptions> options) {
616619
path_ = path;
617620
debug_options_ = options;
618621
client_ = std::make_shared<NodeInspectorClient>(parent_env_);
@@ -626,8 +629,8 @@ bool Agent::Start(const std::string& path, const DebugOptions& options) {
626629
StartDebugSignalHandler();
627630
}
628631

629-
bool wait_for_connect = options.wait_for_connect();
630-
if (!options.inspector_enabled() || !StartIoThread()) {
632+
bool wait_for_connect = options->wait_for_connect();
633+
if (!options->inspector_enabled || !StartIoThread()) {
631634
return false;
632635
}
633636
if (wait_for_connect) {
@@ -789,7 +792,7 @@ void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {
789792
}
790793

791794
bool Agent::WillWaitForConnect() {
792-
return debug_options_.wait_for_connect();
795+
return debug_options_->wait_for_connect();
793796
}
794797

795798
bool Agent::IsActive() {

‎src/inspector_agent.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
#error("This header can only be used when inspector is enabled")
1010
#endif
1111

12-
#include "node_debug_options.h"
12+
#include "node_options.h"
1313
#include "node_persistent.h"
1414
#include "v8.h"
1515

@@ -45,7 +45,7 @@ class Agent {
4545
~Agent();
4646

4747
// Create client_, may create io_ if option enabled
48-
bool Start(const std::string& path, const DebugOptions& options);
48+
bool Start(const std::string& path, std::shared_ptr<DebugOptions> options);
4949
// Stop and destroy io_
5050
void Stop();
5151

@@ -96,7 +96,7 @@ class Agent {
9696
// Calls StartIoThread() from off the main thread.
9797
void RequestIoThreadStart();
9898

99-
DebugOptions& options() { return debug_options_; }
99+
std::shared_ptr<DebugOptions> options() { return debug_options_; }
100100
void ContextCreated(v8::Local<v8::Context> context, const ContextInfo& info);
101101

102102
private:
@@ -109,7 +109,7 @@ class Agent {
109109
// Interface for transports, e.g. WebSocket server
110110
std::unique_ptr<InspectorIo> io_;
111111
std::string path_;
112-
DebugOptions debug_options_;
112+
std::shared_ptr<DebugOptions> debug_options_;
113113

114114
bool pending_enable_async_hook_ = false;
115115
bool pending_disable_async_hook_ = false;

‎src/inspector_io.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ 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-
const DebugOptions& options) {
245+
std::shared_ptr<DebugOptions> options) {
246246
auto io = std::unique_ptr<InspectorIo>(
247247
new InspectorIo(main_thread, path, options));
248248
if (io->request_queue_->Expired()) { // Thread is not running
@@ -253,7 +253,7 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(
253253

254254
InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
255255
const std::string& path,
256-
const DebugOptions& options)
256+
std::shared_ptr<DebugOptions> options)
257257
: main_thread_(main_thread), options_(options),
258258
thread_(), script_name_(path), id_(GenerateID()) {
259259
Mutex::ScopedLock scoped_lock(thread_start_lock_);
@@ -288,7 +288,8 @@ void InspectorIo::ThreadMain() {
288288
new InspectorIoDelegate(queue, main_thread_, id_,
289289
script_path, script_name_));
290290
InspectorSocketServer server(std::move(delegate), &loop,
291-
options_.host_name(), options_.port());
291+
options_->host().c_str(),
292+
options_->port());
292293
request_queue_ = queue->handle();
293294
// Its lifetime is now that of the server delegate
294295
queue.reset();

‎src/inspector_io.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
#define SRC_INSPECTOR_IO_H_
33

44
#include "inspector_socket_server.h"
5-
#include "node_debug_options.h"
65
#include "node_mutex.h"
76
#include "uv.h"
87

@@ -46,19 +45,20 @@ class InspectorIo {
4645
// Returns empty pointer if thread was not started
4746
static std::unique_ptr<InspectorIo> Start(
4847
std::shared_ptr<MainThreadHandle> main_thread, const std::string& path,
49-
const DebugOptions& options);
48+
std::shared_ptr<DebugOptions> options);
5049

5150
// Will block till the transport thread shuts down
5251
~InspectorIo();
5352

5453
void StopAcceptingNewConnections();
55-
std::string host() const { return options_.host_name(); }
54+
const std::string& host() const { return options_->host(); }
5655
int port() const { return port_; }
5756
std::vector<std::string> GetTargetIds() const;
5857

5958
private:
6059
InspectorIo(std::shared_ptr<MainThreadHandle> handle,
61-
const std::string& path, const DebugOptions& options);
60+
const std::string& path,
61+
std::shared_ptr<DebugOptions> options);
6262

6363
// Wrapper for agent->ThreadMain()
6464
static void ThreadMain(void* agent);
@@ -72,7 +72,7 @@ class InspectorIo {
7272
// Used to post on a frontend interface thread, lives while the server is
7373
// running
7474
std::shared_ptr<RequestQueue> request_queue_;
75-
const DebugOptions options_;
75+
std::shared_ptr<DebugOptions> options_;
7676

7777
// The IO thread runs its own uv_loop to implement the TCP server off
7878
// the main thread.

‎src/inspector_js_api.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,12 +242,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {
242242

243243
if (args.Length() > 0 && args[0]->IsUint32()) {
244244
uint32_t port = args[0]->Uint32Value();
245-
agent->options().set_port(static_cast<int>(port));
245+
agent->options()->host_port.port = port;
246246
}
247247

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

253253
if (args.Length() > 2 && args[2]->IsBoolean()) {

0 commit comments

Comments
 (0)