diff --git a/src/env-inl.h b/src/env-inl.h index ba5704ed2e9574..f28701c70f14d2 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -585,6 +585,10 @@ inline std::shared_ptr Environment::options() { return options_; } +inline std::shared_ptr Environment::inspector_host_port() { + return inspector_host_port_; +} + inline std::shared_ptr IsolateData::options() { return options_; } diff --git a/src/env.cc b/src/env.cc index bdb3c1ea247e21..6d69339b585e30 100644 --- a/src/env.cc +++ b/src/env.cc @@ -5,6 +5,7 @@ #include "node_file.h" #include "node_internals.h" #include "node_native_module.h" +#include "node_options-inl.h" #include "node_platform.h" #include "node_worker.h" #include "tracing/agent.h" @@ -192,7 +193,7 @@ Environment::Environment(IsolateData* isolate_data, // 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)); + inspector_host_port_.reset(new HostPort(options_->debug_options().host_port)); #if HAVE_INSPECTOR // We can only create the inspector agent after having cloned the options. diff --git a/src/env.h b/src/env.h index 64593a0025c3b6..ec0368e040a608 100644 --- a/src/env.h +++ b/src/env.h @@ -911,6 +911,7 @@ class Environment { void* data); inline std::shared_ptr options(); + inline std::shared_ptr inspector_host_port(); private: inline void CreateImmediate(native_immediate_callback cb, @@ -942,6 +943,14 @@ class Environment { std::vector destroy_async_id_list_; std::shared_ptr options_; + // options_ contains debug options parsed from CLI arguments, + // while inspector_host_port_ stores the actual inspector host + // and port being used. For example the port is -1 by default + // and can be specified as 0 (meaning any port allocated when the + // server starts listening), but when the inspector server starts + // the inspector_host_port_->port() will be the actual port being + // used. + std::shared_ptr inspector_host_port_; uint32_t module_id_counter_ = 0; uint32_t script_id_counter_ = 0; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 21319abca7d1f5..8949dfe2a7192c 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -669,8 +669,9 @@ class NodeInspectorClient : public V8InspectorClient { }; Agent::Agent(Environment* env) - : parent_env_(env), - debug_options_(env->options()->debug_options) {} + : parent_env_(env), + debug_options_(env->options()->debug_options()), + host_port_(env->inspector_host_port()) {} Agent::~Agent() { if (start_io_thread_async.data == this) { @@ -681,13 +682,14 @@ Agent::~Agent() { } bool Agent::Start(const std::string& path, - std::shared_ptr options, + const DebugOptions& options, + std::shared_ptr host_port, bool is_main) { - if (options == nullptr) { - options = std::make_shared(); - } path_ = path; debug_options_ = options; + CHECK_NE(host_port, nullptr); + host_port_ = host_port; + client_ = std::make_shared(parent_env_, is_main); if (parent_env_->is_main_thread()) { CHECK_EQ(0, uv_async_init(parent_env_->event_loop(), @@ -699,13 +701,18 @@ bool Agent::Start(const std::string& path, StartDebugSignalHandler(); } - bool wait_for_connect = options->wait_for_connect(); + bool wait_for_connect = options.wait_for_connect(); if (parent_handle_) { wait_for_connect = parent_handle_->WaitForConnect(); parent_handle_->WorkerStarted(client_->getThreadHandle(), wait_for_connect); - } else if (!options->inspector_enabled || !StartIoThread()) { + } else if (!options.inspector_enabled || !StartIoThread()) { return false; } + + // TODO(joyeecheung): we should not be using process as a global object + // to transport --inspect-brk. Instead, the JS land can get this through + // require('internal/options') since it should be set once CLI parsing + // is done. if (wait_for_connect) { HandleScope scope(parent_env_->isolate()); parent_env_->process_object()->DefineOwnProperty( @@ -725,8 +732,7 @@ bool Agent::StartIoThread() { CHECK_NOT_NULL(client_); - io_ = InspectorIo::Start( - client_->getThreadHandle(), path_, debug_options_); + io_ = InspectorIo::Start(client_->getThreadHandle(), path_, host_port_); if (io_ == nullptr) { return false; } @@ -867,8 +873,7 @@ void Agent::ContextCreated(Local context, const ContextInfo& info) { } bool Agent::WillWaitForConnect() { - if (debug_options_->wait_for_connect()) - return true; + if (debug_options_.wait_for_connect()) return true; if (parent_handle_) return parent_handle_->WaitForConnect(); return false; diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 9537ae05b61691..5e599a6339e903 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -11,7 +11,7 @@ #error("This header can only be used when inspector is enabled") #endif -#include "node_options.h" +#include "node_options-inl.h" #include "node_persistent.h" #include "v8.h" @@ -50,8 +50,9 @@ class Agent { // Create client_, may create io_ if option enabled bool Start(const std::string& path, - std::shared_ptr options, - bool is_worker); + const DebugOptions& options, + std::shared_ptr host_port, + bool is_main); // Stop and destroy io_ void Stop(); @@ -104,7 +105,8 @@ class Agent { // Calls StartIoThread() from off the main thread. void RequestIoThreadStart(); - std::shared_ptr options() { return debug_options_; } + const DebugOptions& options() { return debug_options_; } + std::shared_ptr host_port() { return host_port_; } void ContextCreated(v8::Local context, const ContextInfo& info); // Interface for interacting with inspectors in worker threads @@ -121,7 +123,13 @@ class Agent { std::unique_ptr io_; std::unique_ptr parent_handle_; std::string path_; - std::shared_ptr debug_options_; + + // This is a copy of the debug options parsed from CLI in the Environment. + // Do not use the host_port in that, instead manipulate the shared host_port_ + // pointer which is meant to store the actual host and port of the inspector + // server. + DebugOptions debug_options_; + std::shared_ptr host_port_; bool pending_enable_async_hook_ = false; bool pending_disable_async_hook_ = false; diff --git a/src/inspector_io.cc b/src/inspector_io.cc index da44d55d06e10a..7686294b2e2842 100644 --- a/src/inspector_io.cc +++ b/src/inspector_io.cc @@ -242,9 +242,9 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate { std::unique_ptr InspectorIo::Start( std::shared_ptr main_thread, const std::string& path, - std::shared_ptr options) { + std::shared_ptr host_port) { auto io = std::unique_ptr( - new InspectorIo(main_thread, path, options)); + new InspectorIo(main_thread, path, host_port)); if (io->request_queue_->Expired()) { // Thread is not running return nullptr; } @@ -253,9 +253,12 @@ std::unique_ptr InspectorIo::Start( InspectorIo::InspectorIo(std::shared_ptr main_thread, const std::string& path, - std::shared_ptr options) - : main_thread_(main_thread), options_(options), - thread_(), script_name_(path), id_(GenerateID()) { + std::shared_ptr host_port) + : main_thread_(main_thread), + host_port_(host_port), + thread_(), + script_name_(path), + id_(GenerateID()) { Mutex::ScopedLock scoped_lock(thread_start_lock_); CHECK_EQ(uv_thread_create(&thread_, InspectorIo::ThreadMain, this), 0); thread_start_condition_.Wait(scoped_lock); @@ -287,16 +290,17 @@ void InspectorIo::ThreadMain() { std::unique_ptr delegate( new InspectorIoDelegate(queue, main_thread_, id_, script_path, script_name_)); - InspectorSocketServer server(std::move(delegate), &loop, - options_->host().c_str(), - options_->port()); + InspectorSocketServer server(std::move(delegate), + &loop, + host_port_->host().c_str(), + host_port_->port()); request_queue_ = queue->handle(); // Its lifetime is now that of the server delegate queue.reset(); { Mutex::ScopedLock scoped_lock(thread_start_lock_); if (server.Start()) { - port_ = server.Port(); + host_port_->set_port(server.Port()); } thread_start_condition_.Broadcast(scoped_lock); } diff --git a/src/inspector_io.h b/src/inspector_io.h index 21df54e03126e9..bc09afdd3df54e 100644 --- a/src/inspector_io.h +++ b/src/inspector_io.h @@ -46,21 +46,22 @@ class InspectorIo { // bool Start(); // Returns empty pointer if thread was not started static std::unique_ptr Start( - std::shared_ptr main_thread, const std::string& path, - std::shared_ptr options); + std::shared_ptr main_thread, + const std::string& path, + std::shared_ptr host_port); // Will block till the transport thread shuts down ~InspectorIo(); void StopAcceptingNewConnections(); - const std::string& host() const { return options_->host(); } - int port() const { return port_; } + const std::string& host() const { return host_port_->host(); } + int port() const { return host_port_->port(); } std::vector GetTargetIds() const; private: InspectorIo(std::shared_ptr handle, const std::string& path, - std::shared_ptr options); + std::shared_ptr host_port); // Wrapper for agent->ThreadMain() static void ThreadMain(void* agent); @@ -74,7 +75,7 @@ class InspectorIo { // Used to post on a frontend interface thread, lives while the server is // running std::shared_ptr request_queue_; - std::shared_ptr options_; + std::shared_ptr host_port_; // The IO thread runs its own uv_loop to implement the TCP server off // the main thread. @@ -84,7 +85,6 @@ class InspectorIo { Mutex thread_start_lock_; ConditionVariable thread_start_condition_; std::string script_name_; - int port_ = -1; // May be accessed from any thread const std::string id_; }; diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 52f14639a430d3..1cb51b51ec09d3 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -243,12 +243,12 @@ void Open(const FunctionCallbackInfo& args) { if (args.Length() > 0 && args[0]->IsUint32()) { uint32_t port = args[0].As()->Value(); - agent->options()->host_port.port = port; + agent->host_port()->set_port(static_cast(port)); } if (args.Length() > 1 && args[1]->IsString()) { Utf8Value host(env->isolate(), args[1].As()); - agent->options()->host_port.host_name = *host; + agent->host_port()->set_host(*host); } if (args.Length() > 2 && args[2]->IsBoolean()) { diff --git a/src/node.cc b/src/node.cc index 25020734430426..82d8653c08ab9e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -27,6 +27,7 @@ #include "node_internals.h" #include "node_metadata.h" #include "node_native_module.h" +#include "node_options-inl.h" #include "node_perf.h" #include "node_platform.h" #include "node_revert.h" @@ -259,13 +260,15 @@ static struct { } #if HAVE_INSPECTOR - bool StartInspector(Environment* env, const char* script_path, - std::shared_ptr options) { + bool StartInspector(Environment* env, const char* script_path) { // Inspector agent can't fail to start, but if it was configured to listen // right away on the websocket port and fails to bind/etc, this will return // false. return env->inspector_agent()->Start( - script_path == nullptr ? "" : script_path, options, true); + script_path == nullptr ? "" : script_path, + env->options()->debug_options(), + env->inspector_host_port(), + true); } bool InspectorStarted(Environment* env) { @@ -306,8 +309,7 @@ static struct { void Dispose() {} void DrainVMTasks(Isolate* isolate) {} void CancelVMTasks(Isolate* isolate) {} - bool StartInspector(Environment* env, const char* script_path, - const DebugOptions& options) { + bool StartInspector(Environment* env, const char* script_path) { env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0"); return true; } @@ -1127,24 +1129,24 @@ void SetupProcessObject(Environment* env, // TODO(refack): move the following 4 to `node_config` // --inspect-brk - if (env->options()->debug_options->wait_for_connect()) { + if (env->options()->debug_options().wait_for_connect()) { READONLY_DONT_ENUM_PROPERTY(process, "_breakFirstLine", True(env->isolate())); } - if (env->options()->debug_options->break_node_first_line) { + if (env->options()->debug_options().break_node_first_line) { READONLY_DONT_ENUM_PROPERTY(process, "_breakNodeFirstLine", True(env->isolate())); } // --inspect --debug-brk - if (env->options()->debug_options->deprecated_invocation()) { + if (env->options()->debug_options().deprecated_invocation()) { READONLY_DONT_ENUM_PROPERTY(process, "_deprecatedDebugBrk", True(env->isolate())); } // --debug or, --debug-brk without --inspect - if (env->options()->debug_options->invalid_invocation()) { + if (env->options()->debug_options().invalid_invocation()) { READONLY_DONT_ENUM_PROPERTY(process, "_invalidDebug", True(env->isolate())); } @@ -1356,7 +1358,7 @@ void LoadEnvironment(Environment* env) { get_linked_binding_fn, get_internal_binding_fn, Boolean::New(env->isolate(), - env->options()->debug_options->break_node_first_line) + env->options()->debug_options().break_node_first_line) }; // Bootstrap internal loaders @@ -1390,12 +1392,10 @@ void LoadEnvironment(Environment* env) { } } - -static void StartInspector(Environment* env, const char* path, - std::shared_ptr debug_options) { +static void StartInspector(Environment* env, const char* path) { #if HAVE_INSPECTOR CHECK(!env->inspector_agent()->IsListening()); - v8_platform.StartInspector(env, path, debug_options); + v8_platform.StartInspector(env, path); #endif // HAVE_INSPECTOR } @@ -2038,9 +2038,9 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.Start(args, exec_args, v8_is_profiling); const char* path = args.size() > 1 ? args[1].c_str() : nullptr; - StartInspector(&env, path, env.options()->debug_options); + StartInspector(&env, path); - if (env.options()->debug_options->inspector_enabled && + if (env.options()->debug_options().inspector_enabled && !v8_platform.InspectorStarted(&env)) { return 12; // Signal internal error. } diff --git a/src/node_config.cc b/src/node_config.cc index c2bf3349c46269..27ec44b8d30538 100644 --- a/src/node_config.cc +++ b/src/node_config.cc @@ -1,5 +1,6 @@ #include "node.h" #include "node_i18n.h" +#include "node_options-inl.h" #include "env-inl.h" #include "util-inl.h" @@ -94,20 +95,18 @@ static void Initialize(Local target, READONLY_STRING_PROPERTY(target, "warningFile", warning_file); } - std::shared_ptr debug_options = env->options()->debug_options; Local debug_options_obj = Object::New(isolate); READONLY_PROPERTY(target, "debugOptions", debug_options_obj); - READONLY_STRING_PROPERTY(debug_options_obj, "host", - debug_options->host()); - - READONLY_PROPERTY(debug_options_obj, - "port", - Integer::New(isolate, debug_options->port())); - + const DebugOptions& debug_options = env->options()->debug_options(); READONLY_PROPERTY(debug_options_obj, "inspectorEnabled", - Boolean::New(isolate, debug_options->inspector_enabled)); + Boolean::New(isolate, debug_options.inspector_enabled)); + READONLY_STRING_PROPERTY( + debug_options_obj, "host", debug_options.host_port.host()); + READONLY_PROPERTY(debug_options_obj, + "port", + Integer::New(isolate, debug_options.host_port.port())); } // InitConfig } // namespace node diff --git a/src/node_options-inl.h b/src/node_options-inl.h index 468368ac84f630..8aaa62524b8ee6 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -14,7 +14,11 @@ PerIsolateOptions* PerProcessOptions::get_per_isolate_options() { } DebugOptions* EnvironmentOptions::get_debug_options() { - return debug_options.get(); + return &debug_options_; +} + +const DebugOptions& EnvironmentOptions::debug_options() const { + return debug_options_; } EnvironmentOptions* PerIsolateOptions::get_per_env_options() { diff --git a/src/node_options.cc b/src/node_options.cc index 7f6ae8bdf009e2..f75010b64e83ed 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -44,7 +44,9 @@ void EnvironmentOptions::CheckOptions(std::vector* errors) { errors->push_back("invalid value for --http-parser"); } - debug_options->CheckOptions(errors); +#if HAVE_INSPECTOR + debug_options_.CheckOptions(errors); +#endif // HAVE_INSPECTOR } namespace options_parser { @@ -54,7 +56,6 @@ namespace options_parser { // TODO(addaleax): Make that unnecessary. DebugOptionsParser::DebugOptionsParser() { -#if HAVE_INSPECTOR AddOption("--inspect-port", "set host:port for inspector", &DebugOptions::host_port, @@ -84,10 +85,11 @@ DebugOptionsParser::DebugOptionsParser() { AddOption("--debug-brk", "", &DebugOptions::break_first_line); Implies("--debug-brk", "--debug"); AddAlias("--debug-brk=", { "--inspect-port", "--debug-brk" }); -#endif } +#if HAVE_INSPECTOR DebugOptionsParser DebugOptionsParser::instance; +#endif // HAVE_INSPECTOR EnvironmentOptionsParser::EnvironmentOptionsParser() { AddOption("--experimental-modules", @@ -203,8 +205,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { AddOption("--napi-modules", "", NoOp{}, kAllowedInEnvironment); +#if HAVE_INSPECTOR Insert(&DebugOptionsParser::instance, &EnvironmentOptions::get_debug_options); +#endif // HAVE_INSPECTOR } EnvironmentOptionsParser EnvironmentOptionsParser::instance; @@ -361,7 +365,7 @@ HostPort SplitHostPort(const std::string& arg, // so if it has an effect only an IPv6 address was specified. std::string host = RemoveBrackets(arg); if (host.length() < arg.length()) - return HostPort { host, -1 }; + return HostPort{host, DebugOptions::kDefaultInspectorPort}; size_t colon = arg.rfind(':'); if (colon == std::string::npos) { @@ -369,7 +373,7 @@ HostPort SplitHostPort(const std::string& arg, // if it's not all decimal digits, it's a host name. for (char c : arg) { if (c < '0' || c > '9') { - return HostPort { arg, -1 }; + return HostPort{arg, DebugOptions::kDefaultInspectorPort}; } } return HostPort { "", ParseAndValidatePort(arg, errors) }; @@ -435,11 +439,11 @@ void GetOptions(const FunctionCallbackInfo& args) { const HostPort& host_port = *parser.Lookup(field, opts); Local obj = Object::New(isolate); Local host; - if (!ToV8Value(context, host_port.host_name).ToLocal(&host) || + if (!ToV8Value(context, host_port.host()).ToLocal(&host) || obj->Set(context, env->host_string(), host).IsNothing() || obj->Set(context, env->port_string(), - Integer::New(isolate, host_port.port)) + Integer::New(isolate, host_port.port())) .IsNothing()) { return; } diff --git a/src/node_options.h b/src/node_options.h index abfd491c98fe7e..a62293286ceb6a 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -3,22 +3,44 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include #include -#include #include -#include +#include #include "node_constants.h" +#include "util.h" namespace node { -struct HostPort { - std::string host_name; - int port; +class HostPort { + public: + HostPort(const std::string& host_name, int port) + : host_name_(host_name), port_(port) {} + HostPort(const HostPort&) = default; + HostPort& operator=(const HostPort&) = default; + HostPort(HostPort&&) = default; + HostPort& operator=(HostPort&&) = default; + + void set_host(const std::string& host) { host_name_ = host; } + + void set_port(int port) { port_ = port; } + + const std::string& host() const { return host_name_; } + + int port() const { + // TODO(joyeecheung): make port a uint16_t + CHECK_GE(port_, 0); + return port_; + } void Update(const HostPort& other) { - if (!other.host_name.empty()) host_name = other.host_name; - if (other.port >= 0) port = other.port; + if (!other.host_name_.empty()) host_name_ = other.host_name_; + if (other.port_ >= 0) port_ = other.port_; } + + private: + std::string host_name_; + int port_; }; class Options { @@ -33,13 +55,25 @@ class Options { // per-Isolate, rather than per-Environment. class DebugOptions : public Options { public: + DebugOptions() = default; + DebugOptions(const DebugOptions&) = default; + DebugOptions& operator=(const DebugOptions&) = default; + DebugOptions(DebugOptions&&) = default; + DebugOptions& operator=(DebugOptions&&) = default; + + // --inspect bool inspector_enabled = false; + // --debug bool deprecated_debug = false; + // --inspect-brk bool break_first_line = false; + // --inspect-brk-node bool break_node_first_line = false; - HostPort host_port = {"127.0.0.1", -1}; + enum { kDefaultInspectorPort = 9229 }; + HostPort host_port{"127.0.0.1", kDefaultInspectorPort}; + bool deprecated_invocation() const { return deprecated_debug && inspector_enabled && @@ -53,19 +87,10 @@ class DebugOptions : public Options { bool wait_for_connect() const { return break_first_line || break_node_first_line; } - - const std::string& host() { - return host_port.host_name; - } - - int port() { - return host_port.port < 0 ? kDefaultInspectorPort : host_port.port; - } }; class EnvironmentOptions : public Options { public: - std::shared_ptr debug_options { new DebugOptions() }; bool abort_on_uncaught_exception = false; bool experimental_modules = false; bool experimental_repl_await = false; @@ -103,7 +128,11 @@ class EnvironmentOptions : public Options { std::vector user_argv; inline DebugOptions* get_debug_options(); + inline const DebugOptions& debug_options() const; void CheckOptions(std::vector* errors); + + private: + DebugOptions debug_options_; }; class PerIsolateOptions : public Options { diff --git a/src/node_process.cc b/src/node_process.cc index a3ccc2343e96e3..792cc510924b4e 100644 --- a/src/node_process.cc +++ b/src/node_process.cc @@ -825,14 +825,7 @@ void GetActiveHandles(const FunctionCallbackInfo& args) { void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); - Mutex::ScopedLock lock(process_mutex); - int port = env->options()->debug_options->port(); -#if HAVE_INSPECTOR - if (port == 0) { - if (auto io = env->inspector_agent()->io()) - port = io->port(); - } -#endif // HAVE_INSPECTOR + int port = env->inspector_host_port()->port(); info.GetReturnValue().Set(port); } @@ -841,9 +834,8 @@ void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); - Mutex::ScopedLock lock(process_mutex); - env->options()->debug_options->host_port.port = - value->Int32Value(env->context()).FromMaybe(0); + int32_t port = value->Int32Value(env->context()).FromMaybe(0); + env->inspector_host_port()->set_port(static_cast(port)); } diff --git a/src/node_worker.cc b/src/node_worker.cc index e99ac59c01e06d..65d3b7297cc65b 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -37,7 +37,10 @@ Mutex next_thread_id_mutex; #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR void StartWorkerInspector(Environment* child, const std::string& url) { - child->inspector_agent()->Start(url, nullptr, false); + child->inspector_agent()->Start(url, + child->options()->debug_options(), + child->inspector_host_port(), + false); } void AddWorkerInspector(Environment* parent,