From e30e307a7003ff37bae9b217c869a39c1a38410c Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Fri, 18 Nov 2016 13:52:22 -0800 Subject: [PATCH] inspector: move options parsing As inspector functionality expands, more options will need to be added. Currently this requires changing adding function arguments, etc. This change packs the veriables into a single class that can be extended without changing APIs. PR-URL: https://github.com/nodejs/node/pull/9691 Reviewed-By: Ben Noordhuis --- node.gyp | 2 + src/debug-agent.cc | 16 ++-- src/debug-agent.h | 6 +- src/inspector_agent.cc | 25 +++--- src/inspector_agent.h | 5 +- src/node.cc | 165 ++++++++------------------------------ src/node_debug_options.cc | 144 +++++++++++++++++++++++++++++++++ src/node_debug_options.h | 51 ++++++++++++ 8 files changed, 260 insertions(+), 154 deletions(-) create mode 100644 src/node_debug_options.cc create mode 100644 src/node_debug_options.h diff --git a/node.gyp b/node.gyp index af10830de5dd8c..3fe0a9830fc2ac 100644 --- a/node.gyp +++ b/node.gyp @@ -159,6 +159,7 @@ 'src/node_config.cc', 'src/node_constants.cc', 'src/node_contextify.cc', + 'src/node_debug_options.cc', 'src/node_file.cc', 'src/node_http_parser.cc', 'src/node_javascript.cc', @@ -201,6 +202,7 @@ 'src/node.h', 'src/node_buffer.h', 'src/node_constants.h', + 'src/node_debug_options.h', 'src/node_file.h', 'src/node_http_parser.h', 'src/node_internals.h', diff --git a/src/debug-agent.cc b/src/debug-agent.cc index 2766702525d088..2d8ed8afc980ec 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -50,7 +50,6 @@ using v8::Value; Agent::Agent(Environment* env) : state_(kNone), - port_(5858), wait_(false), parent_env_(env), child_env_(nullptr), @@ -69,7 +68,7 @@ Agent::~Agent() { } -bool Agent::Start(const char* host, int port, bool wait) { +bool Agent::Start(const DebugOptions& options) { int err; if (state_ == kRunning) @@ -85,9 +84,8 @@ bool Agent::Start(const char* host, int port, bool wait) { goto async_init_failed; uv_unref(reinterpret_cast(&child_signal_)); - host_ = host; - port_ = port; - wait_ = wait; + options_ = options; + wait_ = options_.wait_for_connect(); err = uv_thread_create(&thread_, reinterpret_cast(ThreadCb), @@ -210,9 +208,11 @@ void Agent::InitAdaptor(Environment* env) { api->Set(String::NewFromUtf8(isolate, "host", NewStringType::kNormal).ToLocalChecked(), - String::NewFromUtf8(isolate, host_.data(), NewStringType::kNormal, - host_.size()).ToLocalChecked()); - api->Set(String::NewFromUtf8(isolate, "port"), Integer::New(isolate, port_)); + String::NewFromUtf8(isolate, options_.host_name().data(), + NewStringType::kNormal, + options_.host_name().size()).ToLocalChecked()); + api->Set(String::NewFromUtf8(isolate, "port"), + Integer::New(isolate, options_.port())); env->process_object()->Set(String::NewFromUtf8(isolate, "_debugAPI"), api); api_.Reset(env->isolate(), api); diff --git a/src/debug-agent.h b/src/debug-agent.h index 783403cf0920d1..0faa25f12470a9 100644 --- a/src/debug-agent.h +++ b/src/debug-agent.h @@ -25,6 +25,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include "node_mutex.h" +#include "node_debug_options.h" #include "util.h" #include "util-inl.h" #include "uv.h" @@ -76,7 +77,7 @@ class Agent { typedef void (*DispatchHandler)(node::Environment* env); // Start the debugger agent thread - bool Start(const char* host, int port, bool wait); + bool Start(const DebugOptions& options); // Listen for debug events void Enable(); // Stop the debugger agent @@ -114,9 +115,8 @@ class Agent { }; State state_; + DebugOptions options_; - std::string host_; - int port_; bool wait_; uv_sem_t start_sem_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 8e72b3643c872c..dba4e0f9826004 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -131,7 +131,8 @@ class AgentImpl { explicit AgentImpl(node::Environment* env); // Start the inspector agent thread - bool Start(v8::Platform* platform, const char* path, int port, bool wait); + bool Start(v8::Platform* platform, const char* path, + const DebugOptions& options); // Stop the inspector agent void Stop(); @@ -168,6 +169,7 @@ class AgentImpl { void NotifyMessageReceived(); State ToState(State state); + DebugOptions options_; uv_sem_t start_sem_; ConditionVariable incoming_message_cond_; Mutex state_lock_; @@ -175,8 +177,6 @@ class AgentImpl { uv_loop_t child_loop_; InspectorAgentDelegate* delegate_; - - int port_; bool wait_; bool shutting_down_; State state_; @@ -192,6 +192,8 @@ class AgentImpl { InspectorSocketServer* server_; std::string script_name_; + std::string script_path_; + const std::string id_; friend class ChannelImpl; friend class DispatchOnInspectorBackendTask; @@ -316,7 +318,6 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient { }; AgentImpl::AgentImpl(Environment* env) : delegate_(nullptr), - port_(0), wait_(false), shutting_down_(false), state_(State::kNew), @@ -396,7 +397,10 @@ void InspectorWrapConsoleCall(const v8::FunctionCallbackInfo& args) { } bool AgentImpl::Start(v8::Platform* platform, const char* path, - int port, bool wait) { + const DebugOptions& options) { + options_ = options; + wait_ = options.wait_for_connect(); + auto env = parent_env_; inspector_ = new V8NodeInspector(this, env, platform); platform_ = platform; @@ -408,9 +412,6 @@ bool AgentImpl::Start(v8::Platform* platform, const char* path, int err = uv_loop_init(&child_loop_); CHECK_EQ(err, 0); - port_ = port; - wait_ = wait; - err = uv_thread_create(&thread_, AgentImpl::ThreadCbIO, this); CHECK_EQ(err, 0); uv_sem_wait(&start_sem_); @@ -420,7 +421,7 @@ bool AgentImpl::Start(v8::Platform* platform, const char* path, return false; } state_ = State::kAccepting; - if (wait) { + if (options_.wait_for_connect()) { DispatchMessages(); } return true; @@ -548,7 +549,7 @@ void AgentImpl::WorkerRunIO() { } InspectorAgentDelegate delegate(this, script_path, script_name_, wait_); delegate_ = &delegate; - InspectorSocketServer server(&delegate, port_); + InspectorSocketServer server(&delegate, options_.port()); if (!server.Start(&child_loop_)) { fprintf(stderr, "Unable to open devtools socket: %s\n", uv_strerror(err)); state_ = State::kError; // Safe, main thread is waiting on semaphore @@ -666,8 +667,8 @@ Agent::~Agent() { } bool Agent::Start(v8::Platform* platform, const char* path, - int port, bool wait) { - return impl->Start(platform, path, port, wait); + const DebugOptions& options) { + return impl->Start(platform, path, options); } void Agent::Stop() { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index b31c77496b3d70..9cc2fa676d4d13 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -7,6 +7,8 @@ #error("This header can only be used when inspector is enabled") #endif +#include "node_debug_options.h" + // Forward declaration to break recursive dependency chain with src/env.h. namespace node { class Environment; @@ -31,7 +33,8 @@ class Agent { ~Agent(); // Start the inspector agent thread - bool Start(v8::Platform* platform, const char* path, int port, bool wait); + bool Start(v8::Platform* platform, const char* path, + const DebugOptions& options); // Stop the inspector agent void Stop(); diff --git a/src/node.cc b/src/node.cc index 75f3078db5b4f9..0f1d19283762f8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -7,6 +7,7 @@ #include "node_version.h" #include "node_internals.h" #include "node_revert.h" +#include "node_debug_options.h" #if defined HAVE_PERFCTR #include "node_counters.h" @@ -141,17 +142,6 @@ static bool track_heap_objects = false; static const char* eval_string = nullptr; static unsigned int preload_module_count = 0; static const char** preload_modules = nullptr; -#if HAVE_INSPECTOR -static bool use_inspector = false; -#else -static const bool use_inspector = false; -#endif -static bool use_debug_agent = false; -static bool debug_wait_connect = false; -static std::string* debug_host; // coverity[leaked_storage] -static const int default_debugger_port = 5858; -static const int default_inspector_port = 9229; -static int debug_port = -1; static const int v8_default_thread_pool_size = 4; static int v8_thread_pool_size = v8_default_thread_pool_size; static bool prof_process = false; @@ -198,6 +188,8 @@ static uv_async_t dispatch_debug_messages_async; static Mutex node_isolate_mutex; static v8::Isolate* node_isolate; +static node::DebugOptions debug_options; + static struct { #if NODE_USE_V8_PLATFORM void Initialize(int thread_pool_size) { @@ -214,14 +206,12 @@ static struct { platform_ = nullptr; } - bool StartInspector(Environment *env, const char* script_path, - int port, bool wait) { #if HAVE_INSPECTOR - return env->inspector_agent()->Start(platform_, script_path, port, wait); -#else - return true; -#endif // HAVE_INSPECTOR + bool StartInspector(Environment *env, const char* script_path, + const node::DebugOptions& options) { + return env->inspector_agent()->Start(platform_, script_path, options); } +#endif // HAVE_INSPECTOR v8::Platform* platform_; #else // !NODE_USE_V8_PLATFORM @@ -2522,7 +2512,7 @@ void FatalException(Isolate* isolate, if (exit_code) { #if HAVE_INSPECTOR - if (use_inspector) { + if (debug_options.inspector_enabled()) { env->inspector_agent()->FatalException(error, message); } #endif @@ -2923,17 +2913,14 @@ static Local GetFeatures(Environment* env) { static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { - int port = debug_port; - if (port < 0) - port = use_inspector ? default_inspector_port : default_debugger_port; - info.GetReturnValue().Set(port); + info.GetReturnValue().Set(debug_options.port()); } static void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { - debug_port = value->Int32Value(); + debug_options.set_port(value->Int32Value()); } @@ -3277,7 +3264,7 @@ void SetupProcessObject(Environment* env, } // --debug-brk - if (debug_wait_connect) { + if (debug_options.wait_for_connect()) { READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate())); } @@ -3469,90 +3456,6 @@ void LoadEnvironment(Environment* env) { f->Call(Null(env->isolate()), 1, &arg); } - -static void PrintHelp(); - -static bool ParseDebugOpt(const char* arg) { - const char* port = nullptr; - - if (!strcmp(arg, "--debug")) { - use_debug_agent = true; - } else if (!strncmp(arg, "--debug=", sizeof("--debug=") - 1)) { - use_debug_agent = true; - port = arg + sizeof("--debug=") - 1; - } else if (!strcmp(arg, "--debug-brk")) { - use_debug_agent = true; - debug_wait_connect = true; - } else if (!strncmp(arg, "--debug-brk=", sizeof("--debug-brk=") - 1)) { - use_debug_agent = true; - debug_wait_connect = true; - port = arg + sizeof("--debug-brk=") - 1; - } else if (!strncmp(arg, "--debug-port=", sizeof("--debug-port=") - 1)) { - // XXX(bnoordhuis) Misnomer, configures port and listen address. - port = arg + sizeof("--debug-port=") - 1; -#if HAVE_INSPECTOR - // Specifying both --inspect and --debug means debugging is on, using Chromium - // inspector. - } else if (!strcmp(arg, "--inspect")) { - use_debug_agent = true; - use_inspector = true; - } else if (!strncmp(arg, "--inspect=", sizeof("--inspect=") - 1)) { - use_debug_agent = true; - use_inspector = true; - port = arg + sizeof("--inspect=") - 1; -#else - } else if (!strncmp(arg, "--inspect", sizeof("--inspect") - 1)) { - fprintf(stderr, - "Inspector support is not available with this Node.js build\n"); - return false; -#endif - } else { - return false; - } - - if (port == nullptr) { - return true; - } - - // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js. - // It seems reasonable to support [address]:port notation - // in net.Server#listen() and net.Socket#connect(). - const size_t port_len = strlen(port); - if (port[0] == '[' && port[port_len - 1] == ']') { - debug_host = new std::string(port + 1, port_len - 2); - return true; - } - - const char* const colon = strrchr(port, ':'); - if (colon == nullptr) { - // Either a port number or a host name. Assume that - // if it's not all decimal digits, it's a host name. - for (size_t n = 0; port[n] != '\0'; n += 1) { - if (port[n] < '0' || port[n] > '9') { - debug_host = new std::string(port); - return true; - } - } - } else { - const bool skip = (colon > port && port[0] == '[' && colon[-1] == ']'); - debug_host = new std::string(port + skip, colon - skip); - } - - char* endptr; - errno = 0; - const char* const digits = colon != nullptr ? colon + 1 : port; - const long result = strtol(digits, &endptr, 10); // NOLINT(runtime/int) - if (errno != 0 || *endptr != '\0' || result < 1024 || result > 65535) { - fprintf(stderr, "Debug port must be in range 1024 to 65535.\n"); - PrintHelp(); - exit(12); - } - - debug_port = static_cast(result); - - return true; -} - static void PrintHelp() { // XXX: If you add an option here, please also add it to doc/node.1 and // doc/api/cli.md @@ -3667,8 +3570,8 @@ static void ParseArgs(int* argc, const char* const arg = argv[index]; unsigned int args_consumed = 1; - if (ParseDebugOpt(arg)) { - // Done, consumed by ParseDebugOpt(). + if (debug_options.ParseOption(arg)) { + // Done, consumed by DebugOptions::ParseOption(). } else if (strcmp(arg, "--version") == 0 || strcmp(arg, "-v") == 0) { printf("%s\n", NODE_VERSION); exit(0); @@ -3810,22 +3713,21 @@ static void DispatchMessagesDebugAgentCallback(Environment* env) { } -static void StartDebug(Environment* env, const char* path, bool wait) { +static void StartDebug(Environment* env, const char* path, + DebugOptions debug_options) { CHECK(!debugger_running); - if (use_inspector) { - debugger_running = v8_platform.StartInspector(env, path, - debug_port >= 0 ? debug_port : default_inspector_port, wait); - } else { +#if HAVE_INSPECTOR + if (debug_options.inspector_enabled()) + debugger_running = v8_platform.StartInspector(env, path, debug_options); +#endif // HAVE_INSPECTOR + if (debug_options.debugger_enabled()) { env->debugger_agent()->set_dispatch_handler( DispatchMessagesDebugAgentCallback); - const char* host = debug_host ? debug_host->c_str() : "127.0.0.1"; - int port = debug_port >= 0 ? debug_port : default_debugger_port; - debugger_running = - env->debugger_agent()->Start(host, port, wait); + debugger_running = env->debugger_agent()->Start(debug_options); if (debugger_running == false) { - fprintf(stderr, "Starting debugger on %s:%d failed\n", host, port); + fprintf(stderr, "Starting debugger on %s:%d failed\n", + debug_options.host_name().c_str(), debug_options.port()); fflush(stderr); - return; } } } @@ -3835,7 +3737,7 @@ static void StartDebug(Environment* env, const char* path, bool wait) { static void EnableDebug(Environment* env) { CHECK(debugger_running); - if (use_inspector) { + if (!debug_options.debugger_enabled()) { return; } @@ -3876,8 +3778,8 @@ static void DispatchDebugMessagesAsyncCallback(uv_async_t* handle) { HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); Context::Scope context_scope(env->context()); - - StartDebug(env, nullptr, false); + debug_options.EnableDebugAgent(DebugAgentType::kDebugger); + StartDebug(env, nullptr, debug_options); EnableDebug(env); } @@ -4131,7 +4033,7 @@ static void DebugEnd(const FunctionCallbackInfo& args) { if (debugger_running) { Environment* env = Environment::GetCurrent(args); #if HAVE_INSPECTOR - if (use_inspector) { + if (debug_options.inspector_enabled()) { env->inspector_agent()->Stop(); } else { #endif @@ -4305,7 +4207,7 @@ void Init(int* argc, const char no_typed_array_heap[] = "--typed_array_max_size_in_heap=0"; V8::SetFlagsFromString(no_typed_array_heap, sizeof(no_typed_array_heap) - 1); - if (!use_debug_agent) { + if (!debug_options.debugger_enabled() && !debug_options.inspector_enabled()) { RegisterDebugSignalHandler(); } @@ -4422,11 +4324,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, Environment env(isolate_data, context); env.Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); + bool debug_enabled = + debug_options.debugger_enabled() || debug_options.inspector_enabled(); + // Start debug agent when argv has --debug - if (use_debug_agent) { + if (debug_enabled) { const char* path = argc > 1 ? argv[1] : nullptr; - StartDebug(&env, path, debug_wait_connect); - if (use_inspector && !debugger_running) + StartDebug(&env, path, debug_options); + if (debug_options.debugger_enabled() && !debugger_running) return 12; // Signal internal error. } @@ -4438,7 +4343,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, env.set_trace_sync_io(trace_sync_io); // Enable debugger - if (use_debug_agent) + if (debug_enabled) EnableDebug(&env); { diff --git a/src/node_debug_options.cc b/src/node_debug_options.cc new file mode 100644 index 00000000000000..5681e3d46edaca --- /dev/null +++ b/src/node_debug_options.cc @@ -0,0 +1,144 @@ +#include "node_debug_options.h" + +#include +#include +#include +#include "util.h" + +namespace node { + +namespace { +const int default_debugger_port = 5858; +const int default_inspector_port = 9229; + +inline std::string remove_brackets(const std::string& host) { + if (!host.empty() && host.front() == '[' && host.back() == ']') + return host.substr(1, host.size() - 2); + else + return host; +} + +int parse_and_validate_port(const std::string& port) { + char* endptr; + errno = 0; + const long result = strtol(port.c_str(), &endptr, 10); // NOLINT(runtime/int) + if (errno != 0 || *endptr != '\0'|| result < 1024 || result > 65535) { + fprintf(stderr, "Debug port must be in range 1024 to 65535.\n"); + exit(12); + } + return static_cast(result); +} + +std::pair split_host_port(const std::string& arg) { + // IPv6, no port + std::string host = remove_brackets(arg); + if (host.length() < arg.length()) + return {host, -1}; + + size_t colon = arg.rfind(':'); + if (colon == std::string::npos) { + // Either a port number or a host name. Assume that + // if it's not all decimal digits, it's a host name. + for (char c : arg) { + if (c < '0' || c > '9') { + return {arg, -1}; + } + } + return {"", parse_and_validate_port(arg)}; + } + return std::make_pair(remove_brackets(arg.substr(0, colon)), + parse_and_validate_port(arg.substr(colon + 1))); +} + +} // namespace + +DebugOptions::DebugOptions() : debugger_enabled_(false), +#if HAVE_INSPECTOR + inspector_enabled_(false), +#endif // HAVE_INSPECTOR + wait_connect_(false), http_enabled_(false), + host_name_("127.0.0.1"), port_(-1) { } + +void DebugOptions::EnableDebugAgent(DebugAgentType tool) { + switch (tool) { +#if HAVE_INSPECTOR + case DebugAgentType::kInspector: + inspector_enabled_ = true; + debugger_enabled_ = true; + break; +#endif // HAVE_INSPECTOR + case DebugAgentType::kDebugger: + debugger_enabled_ = true; + break; + case DebugAgentType::kNone: + break; + } +} + +bool DebugOptions::ParseOption(const std::string& option) { + bool enable_inspector = false; + bool has_argument = false; + std::string option_name; + std::string argument; + + auto pos = option.find("="); + if (pos == std::string::npos) { + option_name = option; + } else { + has_argument = true; + option_name = option.substr(0, pos); + argument = option.substr(pos + 1); + } + + if (option_name == "--debug") { + debugger_enabled_ = true; + } else if (option_name == "--debug-brk") { + debugger_enabled_ = true; + wait_connect_ = true; + } else if (option_name == "--inspect") { + debugger_enabled_ = true; + enable_inspector = true; + } else if (option_name != "--debug-port" || !has_argument) { + return false; + } + + if (enable_inspector) { +#if HAVE_INSPECTOR + inspector_enabled_ = true; +#else + fprintf(stderr, + "Inspector support is not available with this Node.js build\n"); + return false; +#endif + } + + if (!has_argument) { + return true; + } + + // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js. + // It seems reasonable to support [address]:port notation + // in net.Server#listen() and net.Socket#connect(). + std::pair host_port = split_host_port(argument); + if (!host_port.first.empty()) { + host_name_ = host_port.first; + } + if (host_port.second >= 0) { + port_ = host_port.second; + } + return true; +} + +int DebugOptions::port() const { + int port = port_; + if (port < 0) { +#if HAVE_INSPECTOR + port = inspector_enabled_ ? default_inspector_port : default_debugger_port; +#else + port = default_debugger_port; +#endif // HAVE_INSPECTOR + } + return port; +} + +} // namespace node diff --git a/src/node_debug_options.h b/src/node_debug_options.h new file mode 100644 index 00000000000000..d03bdb15497bbf --- /dev/null +++ b/src/node_debug_options.h @@ -0,0 +1,51 @@ +#ifndef SRC_NODE_DEBUG_OPTIONS_H_ +#define SRC_NODE_DEBUG_OPTIONS_H_ + +#include + +// Forward declaration to break recursive dependency chain with src/env.h. +namespace node { + +enum class DebugAgentType { + kNone, + kDebugger, +#if HAVE_INSPECTOR + kInspector +#endif // HAVE_INSPECTOR +}; + +class DebugOptions { + public: + DebugOptions(); + bool ParseOption(const std::string& option); + bool debugger_enabled() const { + return debugger_enabled_ && !inspector_enabled(); + } + bool inspector_enabled() const { +#if HAVE_INSPECTOR + return inspector_enabled_; +#else + return false; +#endif // HAVE_INSPECTOR + } + void EnableDebugAgent(DebugAgentType type); + bool ToolsServerEnabled(); + bool wait_for_connect() const { return wait_connect_; } + std::string host_name() const { return host_name_; } + int port() const; + void set_port(int port) { port_ = port; } + + private: + bool debugger_enabled_; +#if HAVE_INSPECTOR + bool inspector_enabled_; +#endif // HAVE_INSPECTOR + bool wait_connect_; + bool http_enabled_; + std::string host_name_; + int port_; +}; + +} // namespace node + +#endif // SRC_NODE_DEBUG_OPTIONS_H_