From d13c544723ec44e08ab96e8dcd80366569e74abf Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Mon, 25 Jul 2016 14:57:43 -0700 Subject: [PATCH] inspector: Do not crash if the port is n/a Node process will no longer terminate with an assertion if the inspector port is not available. --- doc/api/process.md | 5 +-- src/inspector_agent.cc | 80 ++++++++++++++++++++++-------------------- src/inspector_agent.h | 2 +- src/node.cc | 15 +++++--- 4 files changed, 55 insertions(+), 47 deletions(-) diff --git a/doc/api/process.md b/doc/api/process.md index 12eb9b47c66d4d..eec19200065fca 100644 --- a/doc/api/process.md +++ b/doc/api/process.md @@ -1643,8 +1643,9 @@ cases: source code internal in Node.js's bootstrapping process threw an error when the bootstrapping function was called. This is extremely rare, and generally can only happen during development of Node.js itself. -* `12` **Invalid Debug Argument** - The `--debug` and/or `--debug-brk` - options were set, but an invalid port number was chosen. +* `12` **Invalid Debug Argument** - The `--debug`, `--inspect` and/or + `--debug-brk` options were set, but the port number chosen was invalid + or unavailable. * `>128` **Signal Exits** - If Node.js receives a fatal signal such as `SIGKILL` or `SIGHUP`, then its exit code will be `128` plus the value of the signal code. This is a standard Unix practice, since diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 66928e09b114c0..fee459146c3e31 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -165,16 +165,17 @@ class AgentImpl { ~AgentImpl(); // Start the inspector agent thread - void Start(v8::Platform* platform, int port, bool wait); + bool Start(v8::Platform* platform, int port, bool wait); // Stop the inspector agent void Stop(); bool IsStarted(); - bool IsConnected() { return connected_; } + bool IsConnected() { return state_ == State::kConnected; } void WaitForDisconnect(); private: using MessageQueue = std::vector>; + enum class State { kNew, kAccepting, kConnected, kDone, kError }; static void ThreadCbIO(void* agent); static void OnSocketConnectionIO(uv_stream_t* server, int status); @@ -195,6 +196,7 @@ class AgentImpl { const String16& message); void SwapBehindLock(MessageQueue* vector1, MessageQueue* vector2); void PostIncomingMessage(const String16& message); + State ToState(State state); uv_sem_t start_sem_; ConditionVariable pause_cond_; @@ -205,8 +207,8 @@ class AgentImpl { int port_; bool wait_; - bool connected_; bool shutting_down_; + State state_; node::Environment* parent_env_; uv_async_t data_written_; @@ -314,8 +316,8 @@ class V8NodeInspector : public blink::V8Inspector { AgentImpl::AgentImpl(Environment* env) : port_(0), wait_(false), - connected_(false), shutting_down_(false), + state_(State::kNew), parent_env_(env), client_socket_(nullptr), inspector_(nullptr), @@ -334,17 +336,11 @@ AgentImpl::~AgentImpl() { uv_close(reinterpret_cast(&data_written_), nullptr); } -void AgentImpl::Start(v8::Platform* platform, int port, bool wait) { +bool AgentImpl::Start(v8::Platform* platform, int port, bool wait) { auto env = parent_env_; inspector_ = new V8NodeInspector(this, env, platform); - - int err; - platform_ = platform; - - err = uv_loop_init(&child_loop_); - CHECK_EQ(err, 0); - err = uv_async_init(env->event_loop(), &data_written_, nullptr); + int err = uv_async_init(env->event_loop(), &data_written_, nullptr); CHECK_EQ(err, 0); uv_unref(reinterpret_cast(&data_written_)); @@ -356,21 +352,20 @@ void AgentImpl::Start(v8::Platform* platform, int port, bool wait) { CHECK_EQ(err, 0); uv_sem_wait(&start_sem_); + if (state_ == State::kError) { + Stop(); + return false; + } + state_ = State::kAccepting; if (wait) { DispatchMessages(); } + return true; } void AgentImpl::Stop() { - // TODO(repenaxa): hop on the right thread. - DisconnectAndDisposeIO(client_socket_); int err = uv_thread_join(&thread_); CHECK_EQ(err, 0); - - uv_run(&child_loop_, UV_RUN_NOWAIT); - - err = uv_loop_close(&child_loop_); - CHECK_EQ(err, 0); delete inspector_; } @@ -429,7 +424,6 @@ void AgentImpl::OnRemoteDataIO(inspector_socket_t* socket, Mutex::ScopedLock scoped_lock(pause_lock_); if (read > 0) { String16 str = String16::fromUTF8(buf->base, read); - PostIncomingMessage(str); // TODO(pfeldman): Instead of blocking execution while debugger // engages, node should wait for the run callback from the remote client // and initiate its startup. This is a change to node.cc that should be @@ -438,11 +432,7 @@ void AgentImpl::OnRemoteDataIO(inspector_socket_t* socket, wait_ = false; uv_sem_post(&start_sem_); } - - platform_->CallOnForegroundThread(parent_env_->isolate(), - new DispatchOnInspectorBackendTask(this)); - parent_env_->isolate()->RequestInterrupt(InterruptCallback, this); - uv_async_send(&data_written_); + PostIncomingMessage(str); } else if (read <= 0) { // EOF if (client_socket_ == socket) { @@ -477,8 +467,10 @@ void AgentImpl::WriteCbIO(uv_async_t* async) { void AgentImpl::WorkerRunIO() { sockaddr_in addr; uv_tcp_t server; - int err = uv_async_init(&child_loop_, &io_thread_req_, AgentImpl::WriteCbIO); - CHECK_EQ(0, err); + int err = uv_loop_init(&child_loop_); + CHECK_EQ(err, 0); + err = uv_async_init(&child_loop_, &io_thread_req_, AgentImpl::WriteCbIO); + CHECK_EQ(err, 0); io_thread_req_.data = this; uv_tcp_init(&child_loop_, &server); uv_ip4_addr("0.0.0.0", port_, &addr); @@ -489,19 +481,26 @@ void AgentImpl::WorkerRunIO() { err = uv_listen(reinterpret_cast(&server), 1, OnSocketConnectionIO); } - if (err == 0) { - PrintDebuggerReadyMessage(port_); - } else { + if (err != 0) { fprintf(stderr, "Unable to open devtools socket: %s\n", uv_strerror(err)); - ABORT(); + state_ = State::kError; // Safe, main thread is waiting on semaphore + uv_close(reinterpret_cast(&io_thread_req_), nullptr); + uv_close(reinterpret_cast(&server), nullptr); + uv_loop_close(&child_loop_); + uv_sem_post(&start_sem_); + return; } + PrintDebuggerReadyMessage(port_); if (!wait_) { uv_sem_post(&start_sem_); } uv_run(&child_loop_, UV_RUN_DEFAULT); uv_close(reinterpret_cast(&io_thread_req_), nullptr); uv_close(reinterpret_cast(&server), nullptr); - uv_run(&child_loop_, UV_RUN_DEFAULT); + DisconnectAndDisposeIO(client_socket_); + uv_run(&child_loop_, UV_RUN_NOWAIT); + err = uv_loop_close(&child_loop_); + CHECK_EQ(err, 0); } void AgentImpl::AppendMessage(MessageQueue* queue, int session_id, @@ -544,16 +543,19 @@ void AgentImpl::DispatchMessages() { for (const MessageQueue::value_type& pair : tasks) { const String16& message = pair.second; if (message == TAG_CONNECT) { - CHECK_EQ(false, connected_); + CHECK_EQ(State::kAccepting, state_); backend_session_id_++; - connected_ = true; + state_ = State::kConnected; fprintf(stderr, "Debugger attached.\n"); inspector_->connectFrontend(new ChannelImpl(this)); } else if (message == TAG_DISCONNECT) { - CHECK(connected_); - connected_ = false; - if (!shutting_down_) + CHECK_EQ(State::kConnected, state_); + if (shutting_down_) { + state_ = State::kDone; + } else { PrintDebuggerReadyMessage(port_); + state_ = State::kAccepting; + } inspector_->quitMessageLoopOnPause(); inspector_->disconnectFrontend(); } else { @@ -577,8 +579,8 @@ Agent::~Agent() { delete impl; } -void Agent::Start(v8::Platform* platform, int port, bool wait) { - impl->Start(platform, port, wait); +bool Agent::Start(v8::Platform* platform, int port, bool wait) { + return impl->Start(platform, port, wait); } void Agent::Stop() { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 863f1c30c33b63..f2b2c1a187bd91 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -25,7 +25,7 @@ class Agent { ~Agent(); // Start the inspector agent thread - void Start(v8::Platform* platform, int port, bool wait); + bool Start(v8::Platform* platform, int port, bool wait); // Stop the inspector agent void Stop(); diff --git a/src/node.cc b/src/node.cc index 39986593391c47..d16d70265215e3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -208,9 +208,11 @@ static struct { platform_ = nullptr; } - void StartInspector(Environment *env, int port, bool wait) { + bool StartInspector(Environment *env, int port, bool wait) { #if HAVE_INSPECTOR - env->inspector_agent()->Start(platform_, port, wait); + return env->inspector_agent()->Start(platform_, port, wait); +#else + return true; #endif // HAVE_INSPECTOR } @@ -3756,8 +3758,7 @@ static void DispatchMessagesDebugAgentCallback(Environment* env) { static void StartDebug(Environment* env, bool wait) { CHECK(!debugger_running); if (use_inspector) { - v8_platform.StartInspector(env, inspector_port, wait); - debugger_running = true; + debugger_running = v8_platform.StartInspector(env, inspector_port, wait); } else { env->debugger_agent()->set_dispatch_handler( DispatchMessagesDebugAgentCallback); @@ -4383,8 +4384,12 @@ static void StartNodeInstance(void* arg) { ShouldAbortOnUncaughtException); // Start debug agent when argv has --debug - if (instance_data->use_debug_agent()) + if (instance_data->use_debug_agent()) { StartDebug(&env, debug_wait_connect); + if (use_inspector && !debugger_running) { + exit(12); + } + } { Environment::AsyncCallbackScope callback_scope(&env);