inspector: Do not crash if the port is n/a

Eugene Ostroukhov · ofrobots · commit f789eb3106d1 · 2016-07-29T10:59:32.000-07:00
Node process will no longer terminate with an assertion if the inspector port is not available. PR-URL: #7874 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
diff --git 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
@@ -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<std::pair<int, String16>>;
+  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<uv_handle_t*>(&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<uv_handle_t*>(&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<uv_stream_t*>(&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<uv_handle_t*>(&io_thread_req_), nullptr);
+    uv_close(reinterpret_cast<uv_handle_t*>(&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<uv_handle_t*>(&io_thread_req_), nullptr);
   uv_close(reinterpret_cast<uv_handle_t*>(&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
@@ -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
@@ -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);
