inspector: code cleanup

Remove some unused code from the WS server implementation and switch to
smart pointers where possible.

PR-URL: https://github.com/nodejs/node/pull/21070
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

6 files changed, 81 insertions, 162 deletions

diff --git a/src/inspector_io.cc b/src/inspector_io.cc
index 4db26bee27..615fc97592 100644
--- a/src/inspector_io.cc
+++ b/src/inspector_io.cc
@@ -109,8 +109,12 @@ class IoSessionDelegate : public InspectorSessionDelegate {
 // mostly session start, message received, and session end.
 class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
  public:
-  InspectorIoDelegate(InspectorIo* io, const std::string& script_path,
+  InspectorIoDelegate(InspectorIo* io, const std::string& target_id,
+                      const std::string& script_path,
                       const std::string& script_name, bool wait);
+  ~InspectorIoDelegate() {
+    io_->ServerDone();
+  }
   // Calls PostIncomingMessage() with appropriate InspectorAction:
   //   kStartSession
   void StartSession(int session_id, const std::string& target_id) override;
@@ -122,11 +126,8 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
   std::vector<std::string> GetTargetIds() override;
   std::string GetTargetTitle(const std::string& id) override;
   std::string GetTargetUrl(const std::string& id) override;
-  void ServerDone() override {
-    io_->ServerDone();
-  }
 
-  void AssignTransport(InspectorSocketServer* server) {
+  void AssignServer(InspectorSocketServer* server) override {
     server_ = server;
   }
 
@@ -163,11 +164,11 @@ class DispatchMessagesTask : public v8::Task {
 InspectorIo::InspectorIo(Environment* env, v8::Platform* platform,
                          const std::string& path, const DebugOptions& options,
                          bool wait_for_connect)
-                         : options_(options), thread_(), delegate_(nullptr),
-                           state_(State::kNew), parent_env_(env),
-                           thread_req_(), platform_(platform),
+                         : options_(options), thread_(), state_(State::kNew),
+                           parent_env_(env), thread_req_(), platform_(platform),
                            dispatching_messages_(false), script_name_(path),
-                           wait_for_connect_(wait_for_connect), port_(-1) {
+                           wait_for_connect_(wait_for_connect), port_(-1),
+                           id_(GenerateID()) {
   main_thread_req_ = new AsyncAndAgent({uv_async_t(), env->inspector_agent()});
   CHECK_EQ(0, uv_async_init(env->event_loop(), &main_thread_req_->first,
                             InspectorIo::MainThreadReqAsyncCb));
@@ -244,7 +245,7 @@ void InspectorIo::IoThreadAsyncCb(uv_async_t* async) {
       transport->TerminateConnections();
       // Fallthrough
     case TransportAction::kStop:
-      transport->Stop(nullptr);
+      transport->Stop();
       break;
     case TransportAction::kSendMessage:
       transport->Send(session_id,
@@ -271,11 +272,11 @@ void InspectorIo::ThreadMain() {
   err = uv_async_init(&loop, &thread_req_, IoThreadAsyncCb<Transport>);
   CHECK_EQ(err, 0);
   std::string script_path = ScriptPath(&loop, script_name_);
-  InspectorIoDelegate delegate(this, script_path, script_name_,
-                               wait_for_connect_);
-  delegate_ = &delegate;
-  Transport server(&delegate, &loop, options_.host_name(), options_.port());
-  delegate.AssignTransport(&server);
+  auto delegate = std::unique_ptr<InspectorIoDelegate>(
+      new InspectorIoDelegate(this, id_, script_path, script_name_,
+                              wait_for_connect_));
+  Transport server(std::move(delegate), &loop, options_.host_name(),
+                   options_.port());
   TransportAndIo<Transport> queue_transport(&server, this);
   thread_req_.data = &queue_transport;
   if (!server.Start()) {
@@ -291,8 +292,6 @@ void InspectorIo::ThreadMain() {
   uv_run(&loop, UV_RUN_DEFAULT);
   thread_req_.data = nullptr;
   CHECK_EQ(uv_loop_close(&loop), 0);
-  delegate.AssignTransport(nullptr);
-  delegate_ = nullptr;
 }
 
 template <typename ActionType>
@@ -328,7 +327,7 @@ void InspectorIo::PostIncomingMessage(InspectorAction action, int session_id,
 }
 
 std::vector<std::string> InspectorIo::GetTargetIds() const {
-  return delegate_ ? delegate_->GetTargetIds() : std::vector<std::string>();
+  return { id_ };
 }
 
 TransportAction InspectorIo::Attach(int session_id) {
@@ -416,6 +415,7 @@ bool InspectorIo::WaitForFrontendEvent() {
 }
 
 InspectorIoDelegate::InspectorIoDelegate(InspectorIo* io,
+                                         const std::string& target_id,
                                          const std::string& script_path,
                                          const std::string& script_name,
                                          bool wait)
@@ -423,7 +423,7 @@ InspectorIoDelegate::InspectorIoDelegate(InspectorIo* io,
                                            session_id_(0),
                                            script_name_(script_name),
                                            script_path_(script_path),
-                                           target_id_(GenerateID()),
+                                           target_id_(target_id),
                                            waiting_(wait),
                                            server_(nullptr) { }
 
diff --git a/src/inspector_io.h b/src/inspector_io.h
index 05de2d17cd..9d27fc557b 100644
--- a/src/inspector_io.h
+++ b/src/inspector_io.h
@@ -134,7 +134,6 @@ class InspectorIo {
   // and receive a connection if wait_for_connect was requested.
   uv_sem_t thread_start_sem_;
 
-  InspectorIoDelegate* delegate_;
   State state_;
   node::Environment* parent_env_;
 
@@ -161,6 +160,8 @@ class InspectorIo {
   const bool wait_for_connect_;
   int port_;
   std::unordered_map<int, std::unique_ptr<InspectorSession>> sessions_;
+  // May be accessed from any thread
+  const std::string id_;
 
   friend class DispatchMessagesTask;
   friend class IoSessionDelegate;
diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc
index 1350269cf2..dc36359b5c 100644
--- a/src/inspector_socket.cc
+++ b/src/inspector_socket.cc
@@ -22,7 +22,8 @@ namespace inspector {
 
 class TcpHolder {
  public:
-  using Pointer = std::unique_ptr<TcpHolder, void(*)(TcpHolder*)>;
+  static void DisconnectAndDispose(TcpHolder* holder);
+  using Pointer = DeleteFnPtr<TcpHolder, DisconnectAndDispose>;
 
   static Pointer Accept(uv_stream_t* server,
                         InspectorSocket::DelegatePointer delegate);
@@ -41,7 +42,6 @@ class TcpHolder {
   static void OnClosed(uv_handle_t* handle);
   static void OnDataReceivedCb(uv_stream_t* stream, ssize_t nread,
                                const uv_buf_t* buf);
-  static void DisconnectAndDispose(TcpHolder* holder);
   explicit TcpHolder(InspectorSocket::DelegatePointer delegate);
   ~TcpHolder() = default;
   void ReclaimUvBuf(const uv_buf_t* buf, ssize_t read);
@@ -68,14 +68,10 @@ class ProtocolHandler {
   InspectorSocket* inspector() {
     return inspector_;
   }
-
-  static void Shutdown(ProtocolHandler* handler) {
-    handler->Shutdown();
-  }
+  virtual void Shutdown() = 0;
 
  protected:
   virtual ~ProtocolHandler() = default;
-  virtual void Shutdown() = 0;
   int WriteRaw(const std::vector<char>& buffer, uv_write_cb write_cb);
   InspectorSocket::Delegate* delegate();
 
@@ -653,10 +649,10 @@ TcpHolder::Pointer TcpHolder::Accept(
     err = uv_read_start(tcp, allocate_buffer, OnDataReceivedCb);
   }
   if (err == 0) {
-    return { result, DisconnectAndDispose };
+    return TcpHolder::Pointer(result);
   } else {
     delete result;
-    return { nullptr, nullptr };
+    return nullptr;
   }
 }
 
@@ -721,13 +717,14 @@ void TcpHolder::ReclaimUvBuf(const uv_buf_t* buf, ssize_t read) {
   delete[] buf->base;
 }
 
-// Public interface
-InspectorSocket::InspectorSocket()
-    : protocol_handler_(nullptr, ProtocolHandler::Shutdown) { }
-
 InspectorSocket::~InspectorSocket() = default;
 
 // static
+void InspectorSocket::Shutdown(ProtocolHandler* handler) {
+  handler->Shutdown();
+}
+
+// static
 InspectorSocket::Pointer InspectorSocket::Accept(uv_stream_t* server,
                                                  DelegatePointer delegate) {
   auto tcp = TcpHolder::Accept(server, std::move(delegate));
diff --git a/src/inspector_socket.h b/src/inspector_socket.h
index 81b894f95c..ae49d78ff3 100644
--- a/src/inspector_socket.h
+++ b/src/inspector_socket.h
@@ -40,9 +40,10 @@ class InspectorSocket {
   std::string GetHost();
 
  private:
-  InspectorSocket();
+  static void Shutdown(ProtocolHandler*);
+  InspectorSocket() = default;
 
-  std::unique_ptr<ProtocolHandler, void(*)(ProtocolHandler*)> protocol_handler_;
+  DeleteFnPtr<ProtocolHandler, Shutdown> protocol_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(InspectorSocket);
 };
diff --git a/src/inspector_socket_server.cc b/src/inspector_socket_server.cc
index 42082c9c84..d5eb1b75c8 100644
--- a/src/inspector_socket_server.cc
+++ b/src/inspector_socket_server.cc
@@ -156,43 +156,6 @@ std::string FormatWsAddress(const std::string& host, int port,
   return FormatAddress(FormatHostPort(host, port), target_id, include_protocol);
 }
 
-class Closer {
- public:
-  explicit Closer(InspectorSocketServer* server) : server_(server),
-                                                   close_count_(0) { }
-
-  void AddCallback(InspectorSocketServer::ServerCallback callback) {
-    if (callback == nullptr)
-      return;
-    callbacks_.insert(callback);
-  }
-
-  void DecreaseExpectedCount() {
-    --close_count_;
-    NotifyIfDone();
-  }
-
-  void IncreaseExpectedCount() {
-    ++close_count_;
-  }
-
-  void NotifyIfDone() {
-    if (close_count_ == 0) {
-      for (auto callback : callbacks_) {
-        callback(server_);
-      }
-      InspectorSocketServer* server = server_;
-      delete server->closer_;
-      server->closer_ = nullptr;
-    }
-  }
-
- private:
-  InspectorSocketServer* server_;
-  std::set<InspectorSocketServer::ServerCallback> callbacks_;
-  int close_count_;
-};
-
 class SocketSession {
  public:
   SocketSession(InspectorSocketServer* server, int id, int server_port);
@@ -250,45 +213,38 @@ class SocketSession {
 
 class ServerSocket {
  public:
-  static int Listen(InspectorSocketServer* inspector_server,
-                    sockaddr* addr, uv_loop_t* loop);
+  explicit ServerSocket(InspectorSocketServer* server)
+                        : tcp_socket_(uv_tcp_t()), server_(server) {}
+  int Listen(sockaddr* addr, uv_loop_t* loop);
   void Close() {
-    uv_close(reinterpret_cast<uv_handle_t*>(&tcp_socket_),
-             SocketClosedCallback);
+    uv_close(reinterpret_cast<uv_handle_t*>(&tcp_socket_), FreeOnCloseCallback);
   }
   int port() const { return port_; }
 
  private:
-  explicit ServerSocket(InspectorSocketServer* server)
-      : tcp_socket_(uv_tcp_t()), server_(server), port_(-1) {}
   template <typename UvHandle>
   static ServerSocket* FromTcpSocket(UvHandle* socket) {
     return node::ContainerOf(&ServerSocket::tcp_socket_,
                              reinterpret_cast<uv_tcp_t*>(socket));
   }
   static void SocketConnectedCallback(uv_stream_t* tcp_socket, int status);
-  static void SocketClosedCallback(uv_handle_t* tcp_socket);
   static void FreeOnCloseCallback(uv_handle_t* tcp_socket_) {
     delete FromTcpSocket(tcp_socket_);
   }
   int DetectPort();
+  ~ServerSocket() = default;
 
   uv_tcp_t tcp_socket_;
   InspectorSocketServer* server_;
-  int port_;
+  int port_ = -1;
 };
 
-InspectorSocketServer::InspectorSocketServer(SocketServerDelegate* delegate,
-                                             uv_loop_t* loop,
-                                             const std::string& host,
-                                             int port,
-                                             FILE* out) : loop_(loop),
-                                                          delegate_(delegate),
-                                                          host_(host),
-                                                          port_(port),
-                                                          closer_(nullptr),
-                                                          next_session_id_(0),
-                                                          out_(out) {
+InspectorSocketServer::InspectorSocketServer(
+    std::unique_ptr<SocketServerDelegate> delegate, uv_loop_t* loop,
+    const std::string& host, int port, FILE* out)
+    : loop_(loop), delegate_(std::move(delegate)), host_(host), port_(port),
+      next_session_id_(0), out_(out) {
+  delegate_->AssignServer(this);
   state_ = ServerState::kNew;
 }
 
@@ -328,7 +284,7 @@ void InspectorSocketServer::SessionTerminated(int session_id) {
                                 delegate_->GetTargetIds(), out_);
     }
     if (state_ == ServerState::kStopped) {
-      delegate_->ServerDone();
+      delegate_.reset();
     }
   }
 }
@@ -389,7 +345,11 @@ void InspectorSocketServer::SendListResponse(InspectorSocket* socket,
 }
 
 bool InspectorSocketServer::Start() {
+  CHECK_NE(delegate_, nullptr);
   CHECK_EQ(state_, ServerState::kNew);
+  std::unique_ptr<SocketServerDelegate> delegate_holder;
+  // We will return it if startup is successful
+  delegate_.swap(delegate_holder);
   struct addrinfo hints;
   memset(&hints, 0, sizeof(hints));
   hints.ai_flags = AI_NUMERICSERV;
@@ -407,13 +367,13 @@ bool InspectorSocketServer::Start() {
   }
   for (addrinfo* address = req.addrinfo; address != nullptr;
        address = address->ai_next) {
-    err = ServerSocket::Listen(this, address->ai_addr, loop_);
+    auto server_socket = ServerSocketPtr(new ServerSocket(this));
+    err = server_socket->Listen(address->ai_addr, loop_);
+    if (err == 0)
+      server_sockets_.push_back(std::move(server_socket));
   }
   uv_freeaddrinfo(req.addrinfo);
 
-  if (!connected_sessions_.empty()) {
-    return true;
-  }
   // We only show error if we failed to start server on all addresses. We only
   // show one error, for the last address.
   if (server_sockets_.empty()) {
@@ -424,6 +384,7 @@ bool InspectorSocketServer::Start() {
     }
     return false;
   }
+  delegate_.swap(delegate_holder);
   state_ = ServerState::kRunning;
   // getaddrinfo sorts the addresses, so the first port is most relevant.
   PrintDebuggerReadyMessage(host_, server_sockets_[0]->port(),
@@ -431,17 +392,12 @@ bool InspectorSocketServer::Start() {
   return true;
 }
 
-void InspectorSocketServer::Stop(ServerCallback cb) {
+void InspectorSocketServer::Stop() {
   CHECK_EQ(state_, ServerState::kRunning);
-  if (closer_ == nullptr) {
-    closer_ = new Closer(this);
-  }
-  closer_->AddCallback(cb);
-  closer_->IncreaseExpectedCount();
-  state_ = ServerState::kStopping;
-  for (ServerSocket* server_socket : server_sockets_)
-    server_socket->Close();
-  closer_->NotifyIfDone();
+  state_ = ServerState::kStopped;
+  server_sockets_.clear();
+  if (done())
+    delegate_.reset();
 }
 
 void InspectorSocketServer::TerminateConnections() {
@@ -455,28 +411,6 @@ bool InspectorSocketServer::TargetExists(const std::string& id) {
   return found != target_ids.end();
 }
 
-void InspectorSocketServer::ServerSocketListening(ServerSocket* server_socket) {
-  server_sockets_.push_back(server_socket);
-}
-
-void InspectorSocketServer::ServerSocketClosed(ServerSocket* server_socket) {
-  CHECK_EQ(state_, ServerState::kStopping);
-
-  server_sockets_.erase(std::remove(server_sockets_.begin(),
-                                    server_sockets_.end(), server_socket),
-                        server_sockets_.end());
-  if (!server_sockets_.empty())
-    return;
-
-  if (closer_ != nullptr) {
-    closer_->DecreaseExpectedCount();
-  }
-  if (connected_sessions_.empty()) {
-    delegate_->ServerDone();
-  }
-  state_ = ServerState::kStopped;
-}
-
 int InspectorSocketServer::Port() const {
   if (!server_sockets_.empty()) {
     return server_sockets_[0]->port();
@@ -525,6 +459,10 @@ void InspectorSocketServer::Send(int session_id, const std::string& message) {
   }
 }
 
+void InspectorSocketServer::CloseServerSocket(ServerSocket* server) {
+  server->Close();
+}
+
 // InspectorSession tracking
 SocketSession::SocketSession(InspectorSocketServer* server, int id,
                              int server_port)
@@ -569,11 +507,8 @@ int ServerSocket::DetectPort() {
   return err;
 }
 
-// static
-int ServerSocket::Listen(InspectorSocketServer* inspector_server,
-                         sockaddr* addr, uv_loop_t* loop) {
-  ServerSocket* server_socket = new ServerSocket(inspector_server);
-  uv_tcp_t* server = &server_socket->tcp_socket_;
+int ServerSocket::Listen(sockaddr* addr, uv_loop_t* loop) {
+  uv_tcp_t* server = &tcp_socket_;
   CHECK_EQ(0, uv_tcp_init(loop, server));
   int err = uv_tcp_bind(server, addr, 0);
   if (err == 0) {
@@ -582,12 +517,7 @@ int ServerSocket::Listen(InspectorSocketServer* inspector_server,
                     ServerSocket::SocketConnectedCallback);
   }
   if (err == 0) {
-    err = server_socket->DetectPort();
-  }
-  if (err == 0) {
-    inspector_server->ServerSocketListening(server_socket);
-  } else {
-    uv_close(reinterpret_cast<uv_handle_t*>(server), FreeOnCloseCallback);
+    err = DetectPort();
   }
   return err;
 }
@@ -601,13 +531,5 @@ void ServerSocket::SocketConnectedCallback(uv_stream_t* tcp_socket,
     server_socket->server_->Accept(server_socket->port_, tcp_socket);
   }
 }
-
-// static
-void ServerSocket::SocketClosedCallback(uv_handle_t* tcp_socket) {
-  ServerSocket* server_socket = ServerSocket::FromTcpSocket(tcp_socket);
-  server_socket->server_->ServerSocketClosed(server_socket);
-  delete server_socket;
-}
-
 }  // namespace inspector
 }  // namespace node
diff --git a/src/inspector_socket_server.h b/src/inspector_socket_server.h
index f6003c4c4b..b8d98e13a2 100644
--- a/src/inspector_socket_server.h
+++ b/src/inspector_socket_server.h
@@ -16,19 +16,20 @@
 namespace node {
 namespace inspector {
 
-class Closer;
+class InspectorSocketServer;
 class SocketSession;
 class ServerSocket;
 
 class SocketServerDelegate {
  public:
+  virtual void AssignServer(InspectorSocketServer* server) = 0;
   virtual void StartSession(int session_id, const std::string& target_id) = 0;
   virtual void EndSession(int session_id) = 0;
   virtual void MessageReceived(int session_id, const std::string& message) = 0;
   virtual std::vector<std::string> GetTargetIds() = 0;
   virtual std::string GetTargetTitle(const std::string& id) = 0;
   virtual std::string GetTargetUrl(const std::string& id) = 0;
-  virtual void ServerDone() = 0;
+  virtual ~SocketServerDelegate() {}
 };
 
 // HTTP Server, writes messages requested as TransportActions, and responds
@@ -36,8 +37,7 @@ class SocketServerDelegate {
 
 class InspectorSocketServer {
  public:
-  using ServerCallback = void (*)(InspectorSocketServer*);
-  InspectorSocketServer(SocketServerDelegate* delegate,
+  InspectorSocketServer(std::unique_ptr<SocketServerDelegate> delegate,
                         uv_loop_t* loop,
                         const std::string& host,
                         int port,
@@ -49,7 +49,7 @@ class InspectorSocketServer {
 
   // Called by the TransportAction sent with InspectorIo::Write():
   //   kKill and kStop
-  void Stop(ServerCallback callback);
+  void Stop();
   //   kSendMessage
   void Send(int session_id, const std::string& message);
   //   kKill
@@ -58,13 +58,8 @@ class InspectorSocketServer {
   void AcceptSession(int session_id);
   //   kDeclineSession
   void DeclineSession(int session_id);
-
   int Port() const;
 
-  // Server socket lifecycle. There may be multiple sockets
-  void ServerSocketListening(ServerSocket* server_socket);
-  void ServerSocketClosed(ServerSocket* server_socket);
-
   // Session connection lifecycle
   void Accept(int server_port, uv_stream_t* server_socket);
   bool HandleGetRequest(int session_id, const std::string& host,
@@ -76,27 +71,30 @@ class InspectorSocketServer {
     delegate_->MessageReceived(session_id, message);
   }
   SocketSession* Session(int session_id);
+  bool done() const {
+    return server_sockets_.empty() && connected_sessions_.empty();
+  }
 
  private:
+  static void CloseServerSocket(ServerSocket*);
+  using ServerSocketPtr = DeleteFnPtr<ServerSocket, CloseServerSocket>;
+
   void SendListResponse(InspectorSocket* socket, const std::string& host,
                         SocketSession* session);
   bool TargetExists(const std::string& id);
 
   enum class ServerState {kNew, kRunning, kStopping, kStopped};
   uv_loop_t* loop_;
-  SocketServerDelegate* const delegate_;
+  std::unique_ptr<SocketServerDelegate> delegate_;
   const std::string host_;
   int port_;
   std::string path_;
-  std::vector<ServerSocket*> server_sockets_;
-  Closer* closer_;
+  std::vector<ServerSocketPtr> server_sockets_;
   std::map<int, std::pair<std::string, std::unique_ptr<SocketSession>>>
       connected_sessions_;
   int next_session_id_;
   FILE* out_;
   ServerState state_;
-
-  friend class Closer;
 };
 
 }  // namespace inspector