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.
nodejs · addaleax · Aug 10, 2018 · Aug 18, 2018 · Aug 22, 2018 · Aug 22, 2018
commit 5d123558d6cd8140261ba208fd8db74c83f7537e
diff --git a/node.gyp b/node.gyp
@@ -344,14 +344,14 @@
         'src/node_config.cc',
         'src/node_constants.cc',
         'src/node_contextify.cc',
-        'src/node_debug_options.cc',
         'src/node_domain.cc',
         'src/node_encoding.cc',
         'src/node_errors.h',
         'src/node_file.cc',
         'src/node_http2.cc',
         'src/node_http_parser.cc',
         'src/node_messaging.cc',
+        'src/node_options.cc',
         'src/node_os.cc',
         'src/node_platform.cc',
         'src/node_perf.cc',
@@ -408,14 +408,15 @@
         'src/node_code_cache.h',
         'src/node_constants.h',
         'src/node_contextify.h',
-        'src/node_debug_options.h',
         'src/node_file.h',
         'src/node_http2.h',
         'src/node_http2_state.h',
         'src/node_internals.h',
         'src/node_javascript.h',
         'src/node_messaging.h',
         'src/node_mutex.h',
+        'src/node_options.h',
+        'src/node_options-inl.h',
         'src/node_perf.h',
         'src/node_perf_common.h',
         'src/node_persistent.h',

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -564,6 +564,14 @@ Environment::file_handle_read_wrap_freelist() {
   return file_handle_read_wrap_freelist_;
 }
 
+inline std::shared_ptr<EnvironmentOptions> Environment::options() {
+  return options_;
+}
+
+inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
+  return options_;
+}
+
 void Environment::CreateImmediate(native_immediate_callback cb,
                                   void* data,
                                   v8::Local<v8::Object> obj,

diff --git a/src/env.cc b/src/env.cc
@@ -48,6 +48,8 @@ IsolateData::IsolateData(Isolate* isolate,
   if (platform_ != nullptr)
     platform_->RegisterIsolate(this, event_loop);
 
+  options_.reset(new PerIsolateOptions(*per_process_opts->per_isolate));
+
   // Create string and private symbol properties as internalized one byte
   // strings after the platform is properly initialized.
   //
@@ -136,9 +138,6 @@ Environment::Environment(IsolateData* isolate_data,
       makecallback_cntr_(0),
       should_abort_on_uncaught_toggle_(isolate_, 1),
       trace_category_state_(isolate_, kTraceCategoryCount),
-#if HAVE_INSPECTOR
-      inspector_agent_(new inspector::Agent(this)),
-#endif
       http_parser_buffer_(nullptr),
       fs_stats_field_array_(isolate_, kFsStatsFieldsLength * 2),
       fs_stats_field_bigint_array_(isolate_, kFsStatsFieldsLength * 2),
@@ -148,6 +147,19 @@ Environment::Environment(IsolateData* isolate_data,
   v8::Context::Scope context_scope(context);
   set_as_external(v8::External::New(isolate(), this));
 
+  // We create new copies of the per-Environment option sets, so that it is
+  // easier to modify them after Environment creation. The defaults are
+  // part of the per-Isolate option set, for which in turn the defaults are
+  // part of the per-process option set.
+  options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env));
+  options_->debug_options.reset(new DebugOptions(*options_->debug_options));
+
+#if HAVE_INSPECTOR
+  // We can only create the inspector agent after having cloned the options.
+  inspector_agent_ =
+      std::unique_ptr<inspector::Agent>(new inspector::Agent(this));
+#endif
+
   AssignToContext(context, ContextInfo(""));
 
   if (tracing_agent_writer_ != nullptr) {
@@ -211,10 +223,8 @@ Environment::~Environment() {
   delete[] http_parser_buffer_;
 }
 
-void Environment::Start(int argc,
-                        const char* const* argv,
-                        int exec_argc,
-                        const char* const* exec_argv,
+void Environment::Start(const std::vector<std::string>& args,
+                        const std::vector<std::string>& exec_args,
                         bool start_profiler_idle_notifier) {
   HandleScope handle_scope(isolate());
   Context::Scope context_scope(context());
@@ -260,7 +270,7 @@ void Environment::Start(int argc,
       process_template->GetFunction()->NewInstance(context()).ToLocalChecked();
   set_process_object(process_object);
 
-  SetupProcessObject(this, argc, argv, exec_argc, exec_argv);
+  SetupProcessObject(this, args, exec_args);
 
   static uv_once_t init_once = UV_ONCE_INIT;
   uv_once(&init_once, InitThreadLocalOnce);

diff --git a/src/env.h b/src/env.h
@@ -34,6 +34,7 @@
 #include "uv.h"
 #include "v8.h"
 #include "node.h"
+#include "node_options.h"
 #include "node_http2_state.h"
 
 #include <list>
@@ -361,6 +362,7 @@ class IsolateData {
   inline uv_loop_t* event_loop() const;
   inline uint32_t* zero_fill_field() const;
   inline MultiIsolatePlatform* platform() const;
+  inline std::shared_ptr<PerIsolateOptions> options();
 
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
 #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
@@ -394,6 +396,7 @@ class IsolateData {
   uv_loop_t* const event_loop_;
   uint32_t* const zero_fill_field_;
   MultiIsolatePlatform* platform_;
+  std::shared_ptr<PerIsolateOptions> options_;
 
   DISALLOW_COPY_AND_ASSIGN(IsolateData);
 };
@@ -578,10 +581,8 @@ class Environment {
               tracing::AgentWriterHandle* tracing_agent_writer);
   ~Environment();
 
-  void Start(int argc,
-             const char* const* argv,
-             int exec_argc,
-             const char* const* exec_argv,
+  void Start(const std::vector<std::string>& args,
+             const std::vector<std::string>& exec_args,
              bool start_profiler_idle_notifier);
 
   typedef void (*HandleCleanupCb)(Environment* env,
@@ -878,6 +879,8 @@ class Environment {
                                  v8::EmbedderGraph* graph,
                                  void* data);
 
+  inline std::shared_ptr<EnvironmentOptions> options();
+
  private:
   inline void CreateImmediate(native_immediate_callback cb,
                               void* data,
@@ -908,6 +911,8 @@ class Environment {
   size_t makecallback_cntr_;
   std::vector<double> destroy_async_id_list_;
 
+  std::shared_ptr<EnvironmentOptions> options_;
+
   AliasedBuffer<uint32_t, v8::Uint32Array> should_abort_on_uncaught_toggle_;
   int should_not_abort_scope_counter_ = 0;
 

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -608,11 +608,14 @@ class NodeInspectorClient : public V8InspectorClient {
   std::unique_ptr<MainThreadInterface> interface_;
 };
 
-Agent::Agent(Environment* env) : parent_env_(env) {}
+Agent::Agent(Environment* env)
+  : parent_env_(env),
+    debug_options_(env->options()->debug_options) {}
 
 Agent::~Agent() = default;
 
-bool Agent::Start(const std::string& path, const DebugOptions& options) {
+bool Agent::Start(const std::string& path,
+                  std::shared_ptr<DebugOptions> options) {
   path_ = path;
   debug_options_ = options;
   client_ = std::make_shared<NodeInspectorClient>(parent_env_);
@@ -626,8 +629,8 @@ bool Agent::Start(const std::string& path, const DebugOptions& options) {
     StartDebugSignalHandler();
   }
 
-  bool wait_for_connect = options.wait_for_connect();
-  if (!options.inspector_enabled() || !StartIoThread()) {
+  bool wait_for_connect = options->wait_for_connect();
+  if (!options->inspector_enabled || !StartIoThread()) {
     return false;
   }
   if (wait_for_connect) {
@@ -789,7 +792,7 @@ void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {
 }
 
 bool Agent::WillWaitForConnect() {
-  return debug_options_.wait_for_connect();
+  return debug_options_->wait_for_connect();
 }
 
 bool Agent::IsActive() {

diff --git a/src/inspector_agent.h b/src/inspector_agent.h
@@ -9,7 +9,7 @@
 #error("This header can only be used when inspector is enabled")
 #endif
 
-#include "node_debug_options.h"
+#include "node_options.h"
 #include "node_persistent.h"
 #include "v8.h"
 
@@ -45,7 +45,7 @@ class Agent {
   ~Agent();
 
   // Create client_, may create io_ if option enabled
-  bool Start(const std::string& path, const DebugOptions& options);
+  bool Start(const std::string& path, std::shared_ptr<DebugOptions> options);
   // Stop and destroy io_
   void Stop();
 
@@ -96,7 +96,7 @@ class Agent {
   // Calls StartIoThread() from off the main thread.
   void RequestIoThreadStart();
 
-  DebugOptions& options() { return debug_options_; }
+  std::shared_ptr<DebugOptions> options() { return debug_options_; }
   void ContextCreated(v8::Local<v8::Context> context, const ContextInfo& info);
 
  private:
@@ -109,7 +109,7 @@ class Agent {
   // Interface for transports, e.g. WebSocket server
   std::unique_ptr<InspectorIo> io_;
   std::string path_;
-  DebugOptions debug_options_;
+  std::shared_ptr<DebugOptions> debug_options_;
 
   bool pending_enable_async_hook_ = false;
   bool pending_disable_async_hook_ = false;

diff --git a/src/inspector_io.cc b/src/inspector_io.cc
@@ -242,7 +242,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
 std::unique_ptr<InspectorIo> InspectorIo::Start(
     std::shared_ptr<MainThreadHandle> main_thread,
     const std::string& path,
-    const DebugOptions& options) {
+    std::shared_ptr<DebugOptions> options) {
   auto io = std::unique_ptr<InspectorIo>(
       new InspectorIo(main_thread, path, options));
   if (io->request_queue_->Expired()) {  // Thread is not running
@@ -253,7 +253,7 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(
 
 InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
                          const std::string& path,
-                         const DebugOptions& options)
+                         std::shared_ptr<DebugOptions> options)
                          : main_thread_(main_thread), options_(options),
                            thread_(), script_name_(path), id_(GenerateID()) {
   Mutex::ScopedLock scoped_lock(thread_start_lock_);
@@ -288,7 +288,8 @@ void InspectorIo::ThreadMain() {
       new InspectorIoDelegate(queue, main_thread_, id_,
                               script_path, script_name_));
   InspectorSocketServer server(std::move(delegate), &loop,
-                               options_.host_name(), options_.port());
+                               options_->host().c_str(),
+                               options_->port());
   request_queue_ = queue->handle();
   // Its lifetime is now that of the server delegate
   queue.reset();

diff --git a/src/inspector_io.h b/src/inspector_io.h
@@ -2,7 +2,6 @@
 #define SRC_INSPECTOR_IO_H_
 
 #include "inspector_socket_server.h"
-#include "node_debug_options.h"
 #include "node_mutex.h"
 #include "uv.h"
 
@@ -46,19 +45,20 @@ class InspectorIo {
   // Returns empty pointer if thread was not started
   static std::unique_ptr<InspectorIo> Start(
       std::shared_ptr<MainThreadHandle> main_thread, const std::string& path,
-      const DebugOptions& options);
+      std::shared_ptr<DebugOptions> options);
 
   // Will block till the transport thread shuts down
   ~InspectorIo();
 
   void StopAcceptingNewConnections();
-  std::string host() const { return options_.host_name(); }
+  const std::string& host() const { return options_->host(); }
   int port() const { return port_; }
   std::vector<std::string> GetTargetIds() const;
 
  private:
   InspectorIo(std::shared_ptr<MainThreadHandle> handle,
-              const std::string& path, const DebugOptions& options);
+              const std::string& path,
+              std::shared_ptr<DebugOptions> options);
 
   // Wrapper for agent->ThreadMain()
   static void ThreadMain(void* agent);
@@ -72,7 +72,7 @@ class InspectorIo {
   // Used to post on a frontend interface thread, lives while the server is
   // running
   std::shared_ptr<RequestQueue> request_queue_;
-  const DebugOptions options_;
+  std::shared_ptr<DebugOptions> options_;
 
   // The IO thread runs its own uv_loop to implement the TCP server off
   // the main thread.

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -242,12 +242,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {
 
   if (args.Length() > 0 && args[0]->IsUint32()) {
     uint32_t port = args[0]->Uint32Value();
-    agent->options().set_port(static_cast<int>(port));
+    agent->options()->host_port.port = port;
   }
 
   if (args.Length() > 1 && args[1]->IsString()) {
     Utf8Value host(env->isolate(), args[1].As<String>());
-    agent->options().set_host_name(*host);
+    agent->options()->host_port.host_name = *host;
   }
 
   if (args.Length() > 2 && args[2]->IsBoolean()) {