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>

7 files changed, 229 insertions, 337 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
diff --git a/test/cctest/test_inspector_socket_server.cc b/test/cctest/test_inspector_socket_server.cc
index 9023ad43c1..60b7eefc5e 100644
--- a/test/cctest/test_inspector_socket_server.cc
+++ b/test/cctest/test_inspector_socket_server.cc
@@ -84,78 +84,6 @@ class InspectorSocketServerTest : public ::testing::Test {
   }
 };
 
-class TestInspectorServerDelegate : public SocketServerDelegate {
- public:
-  TestInspectorServerDelegate() : connected(0), disconnected(0), done(false),
-                                  targets_({ MAIN_TARGET_ID,
-                                             UNCONNECTABLE_TARGET_ID }),
-                                  session_id_(0) {}
-
-  void Connect(InspectorSocketServer* server) {
-    server_ = server;
-  }
-
-  void StartSession(int session_id, const std::string& target_id) override {
-    buffer_.clear();
-    CHECK_NE(targets_.end(),
-             std::find(targets_.begin(), targets_.end(), target_id));
-    if (target_id == UNCONNECTABLE_TARGET_ID) {
-      server_->DeclineSession(session_id);
-      return;
-    }
-    connected++;
-    session_id_ = session_id;
-    server_->AcceptSession(session_id);
-  }
-
-  void MessageReceived(int session_id, const std::string& message) override {
-    CHECK_EQ(session_id_, session_id);
-    buffer_.insert(buffer_.end(), message.begin(), message.end());
-  }
-
-  void EndSession(int session_id) override {
-    CHECK_EQ(session_id_, session_id);
-    disconnected++;
-  }
-
-  std::vector<std::string> GetTargetIds() override {
-    return targets_;
-  }
-
-  std::string GetTargetTitle(const std::string& id) override {
-    return id + " Target Title";
-  }
-
-  std::string GetTargetUrl(const std::string& id) override {
-    return "file://" + id + "/script.js";
-  }
-
-  void Expect(const std::string& expects) {
-    SPIN_WHILE(buffer_.size() < expects.length());
-    ASSERT_STREQ(std::string(buffer_.data(), expects.length()).c_str(),
-                 expects.c_str());
-    buffer_.erase(buffer_.begin(), buffer_.begin() + expects.length());
-  }
-
-  void Write(const std::string& message) {
-    server_->Send(session_id_, message);
-  }
-
-  void ServerDone() override {
-    done = true;
-  }
-
-  int connected;
-  int disconnected;
-  bool done;
-
- private:
-  const std::vector<std::string> targets_;
-  InspectorSocketServer* server_;
-  int session_id_;
-  std::vector<char> buffer_;
-};
-
 class SocketWrapper {
  public:
   explicit SocketWrapper(uv_loop_t* loop) : closed_(false),
@@ -312,76 +240,136 @@ class SocketWrapper {
 
 class ServerHolder {
  public:
-  template <typename Delegate>
-  ServerHolder(Delegate* delegate, uv_loop_t* loop, int port)
-               : ServerHolder(delegate, loop, HOST, port, nullptr) { }
+  ServerHolder(bool has_targets, uv_loop_t* loop, int port)
+               : ServerHolder(has_targets, loop, HOST, port, nullptr) { }
 
-  template <typename Delegate>
-  ServerHolder(Delegate* delegate, uv_loop_t* loop, const std::string host,
-               int port, FILE* out)
-               : closed(false), paused(false),
-                 server_(delegate, loop, host, port, out) {
-    delegate->Connect(&server_);
-  }
+  ServerHolder(bool has_targets, uv_loop_t* loop,
+               const std::string host, int port, FILE* out);
 
   InspectorSocketServer* operator->() {
-    return &server_;
+    return server_.get();
   }
 
   int port() {
-    return server_.Port();
+    return server_->Port();
   }
 
-  static void CloseCallback(InspectorSocketServer* server) {
-    ServerHolder* holder = node::ContainerOf(&ServerHolder::server_, server);
-    holder->closed = true;
+  bool done() {
+    return server_->done();
   }
 
-  static void PausedCallback(InspectorSocketServer* server) {
-    ServerHolder* holder = node::ContainerOf(&ServerHolder::server_, server);
-    holder->paused = true;
+  void Connected() {
+    connected++;
+  }
+
+  void Disconnected() {
+    disconnected++;
+  }
+
+  void Done() {
+    delegate_done = true;
+  }
+
+  void PrepareSession(int id) {
+    buffer_.clear();
+    session_id_ = id;
+  }
+
+  void Received(const std::string& message) {
+    buffer_.insert(buffer_.end(), message.begin(), message.end());
   }
 
-  bool closed;
-  bool paused;
+  void Write(const std::string& message) {
+    server_->Send(session_id_, message);
+  }
+
+  void Expect(const std::string& expects) {
+    SPIN_WHILE(buffer_.size() < expects.length());
+    ASSERT_STREQ(std::string(buffer_.data(), expects.length()).c_str(),
+                 expects.c_str());
+    buffer_.erase(buffer_.begin(), buffer_.begin() + expects.length());
+  }
+
+  int connected = 0;
+  int disconnected = 0;
+  bool delegate_done = false;
 
  private:
-  InspectorSocketServer server_;
+  std::unique_ptr<InspectorSocketServer> server_;
+  std::vector<char> buffer_;
+  int session_id_;
 };
 
-class ServerDelegateNoTargets : public SocketServerDelegate {
+class TestSocketServerDelegate : public SocketServerDelegate {
  public:
-  ServerDelegateNoTargets() : server_(nullptr) { }
-  void Connect(InspectorSocketServer* server) { }
-  void MessageReceived(int session_id, const std::string& message) override { }
-  void EndSession(int session_id) override { }
+  explicit TestSocketServerDelegate(
+      ServerHolder* server,
+      const std::vector<std::string>& target_ids)
+      : harness_(server),
+        targets_(target_ids),
+        session_id_(0) {}
+
+  ~TestSocketServerDelegate() {
+    harness_->Done();
+  }
+
+  void AssignServer(InspectorSocketServer* server) override {
+    server_ = server;
+  }
 
   void StartSession(int session_id, const std::string& target_id) override {
-    server_->DeclineSession(session_id);
+    session_id_ = session_id;
+    harness_->PrepareSession(session_id_);
+    CHECK_NE(targets_.end(),
+             std::find(targets_.begin(), targets_.end(), target_id));
+    if (target_id == UNCONNECTABLE_TARGET_ID) {
+      server_->DeclineSession(session_id);
+      return;
+    }
+    harness_->Connected();
+    server_->AcceptSession(session_id);
   }
 
-  std::vector<std::string> GetTargetIds() override {
-    return std::vector<std::string>();
+  void MessageReceived(int session_id, const std::string& message) override {
+    CHECK_EQ(session_id_, session_id);
+    harness_->Received(message);
   }
 
-  std::string GetTargetTitle(const std::string& id) override {
-    return "";
+  void EndSession(int session_id) override {
+    CHECK_EQ(session_id_, session_id);
+    harness_->Disconnected();
   }
 
-  std::string GetTargetUrl(const std::string& id) override {
-    return "";
+  std::vector<std::string> GetTargetIds() override {
+    return targets_;
   }
 
-  void ServerDone() override {
-    done = true;
+  std::string GetTargetTitle(const std::string& id) override {
+    return id + " Target Title";
   }
 
-  bool done = false;
+  std::string GetTargetUrl(const std::string& id) override {
+    return "file://" + id + "/script.js";
+  }
 
  private:
+  ServerHolder* harness_;
+  const std::vector<std::string> targets_;
   InspectorSocketServer* server_;
+  int session_id_;
 };
 
+ServerHolder::ServerHolder(bool has_targets, uv_loop_t* loop,
+                           const std::string host, int port, FILE* out) {
+  std::vector<std::string> targets;
+  if (has_targets)
+    targets = { MAIN_TARGET_ID, UNCONNECTABLE_TARGET_ID };
+  std::unique_ptr<TestSocketServerDelegate> delegate(
+      new TestSocketServerDelegate(this, targets));
+  server_.reset(
+      new InspectorSocketServer(std::move(delegate), loop, host, port, out));
+}
+
 static void TestHttpRequest(int port, const std::string& path,
                             const std::string& expected_body) {
   SocketWrapper socket(&loop);
@@ -402,8 +390,7 @@ static const std::string WsHandshakeRequest(const std::string& target_id) {
 
 
 TEST_F(InspectorSocketServerTest, InspectorSessions) {
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(true, &loop, 0);
   ASSERT_TRUE(server->Start());
 
   SocketWrapper well_behaved_socket(&loop);
@@ -412,18 +399,18 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   well_behaved_socket.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   well_behaved_socket.Expect(WS_HANDSHAKE_RESPONSE);
 
-  EXPECT_EQ(1, delegate.connected);
+  EXPECT_EQ(1, server.connected);
 
   well_behaved_socket.Write("\x81\x84\x7F\xC2\x66\x31\x4E\xF0\x55\x05");
 
-  delegate.Expect("1234");
-  delegate.Write("5678");
+  server.Expect("1234");
+  server.Write("5678");
 
   well_behaved_socket.Expect("\x81\x4" "5678");
   well_behaved_socket.Write(CLIENT_CLOSE_FRAME);
   well_behaved_socket.Expect(SERVER_CLOSE_FRAME);
 
-  EXPECT_EQ(1, delegate.disconnected);
+  EXPECT_EQ(1, server.disconnected);
 
   well_behaved_socket.Close();
 
@@ -433,8 +420,8 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   declined_target_socket.Write(WsHandshakeRequest(UNCONNECTABLE_TARGET_ID));
   declined_target_socket.Expect("HTTP/1.0 400 Bad Request");
   declined_target_socket.ExpectEOF();
-  EXPECT_EQ(1, delegate.connected);
-  EXPECT_EQ(1, delegate.disconnected);
+  EXPECT_EQ(1, server.connected);
+  EXPECT_EQ(1, server.disconnected);
 
   // Bogus target - start session callback should not even be invoked
   SocketWrapper bogus_target_socket(&loop);
@@ -442,8 +429,8 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   bogus_target_socket.Write(WsHandshakeRequest("bogus_target"));
   bogus_target_socket.Expect("HTTP/1.0 400 Bad Request");
   bogus_target_socket.ExpectEOF();
-  EXPECT_EQ(1, delegate.connected);
-  EXPECT_EQ(1, delegate.disconnected);
+  EXPECT_EQ(1, server.connected);
+  EXPECT_EQ(1, server.disconnected);
 
   // Drop connection (no proper close frames)
   SocketWrapper dropped_connection_socket(&loop);
@@ -451,13 +438,13 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   dropped_connection_socket.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   dropped_connection_socket.Expect(WS_HANDSHAKE_RESPONSE);
 
-  EXPECT_EQ(2, delegate.connected);
+  EXPECT_EQ(2, server.connected);
 
-  delegate.Write("5678");
+  server.Write("5678");
   dropped_connection_socket.Expect("\x81\x4" "5678");
 
   dropped_connection_socket.Close();
-  SPIN_WHILE(delegate.disconnected < 2);
+  SPIN_WHILE(server.disconnected < 2);
 
   // Reconnect regular connection
   SocketWrapper stays_till_termination_socket(&loop);
@@ -465,41 +452,38 @@ TEST_F(InspectorSocketServerTest, InspectorSessions) {
   stays_till_termination_socket.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   stays_till_termination_socket.Expect(WS_HANDSHAKE_RESPONSE);
 
-  SPIN_WHILE(3 != delegate.connected);
+  SPIN_WHILE(3 != server.connected);
 
-  delegate.Write("5678");
+  server.Write("5678");
   stays_till_termination_socket.Expect("\x81\x4" "5678");
 
   stays_till_termination_socket
       .Write("\x81\x84\x7F\xC2\x66\x31\x4E\xF0\x55\x05");
-  delegate.Expect("1234");
+  server.Expect("1234");
 
-  server->Stop(ServerHolder::CloseCallback);
+  server->Stop();
   server->TerminateConnections();
 
   stays_till_termination_socket.Write(CLIENT_CLOSE_FRAME);
   stays_till_termination_socket.Expect(SERVER_CLOSE_FRAME);
 
-  SPIN_WHILE(3 != delegate.disconnected);
-
-  SPIN_WHILE(!server.closed);
+  SPIN_WHILE(3 != server.disconnected);
+  SPIN_WHILE(!server.done());
   stays_till_termination_socket.ExpectEOF();
 }
 
 TEST_F(InspectorSocketServerTest, ServerDoesNothing) {
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(true, &loop, 0);
   ASSERT_TRUE(server->Start());
-
-  server->Stop(ServerHolder::CloseCallback);
+  server->Stop();
   server->TerminateConnections();
-  SPIN_WHILE(!server.closed);
-  ASSERT_TRUE(delegate.done);
+  SPIN_WHILE(!server.done());
+  ASSERT_TRUE(server.delegate_done);
+  SPIN_WHILE(uv_loop_alive(&loop));
 }
 
 TEST_F(InspectorSocketServerTest, ServerWithoutTargets) {
-  ServerDelegateNoTargets delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(false, &loop, 0);
   ASSERT_TRUE(server->Start());
   TestHttpRequest(server.port(), "/json/list", "[ ]");
   TestHttpRequest(server.port(), "/json", "[ ]");
@@ -510,89 +494,81 @@ TEST_F(InspectorSocketServerTest, ServerWithoutTargets) {
   socket.Write(WsHandshakeRequest(UNCONNECTABLE_TARGET_ID));
   socket.Expect("HTTP/1.0 400 Bad Request");
   socket.ExpectEOF();
-  server->Stop(ServerHolder::CloseCallback);
+  server->Stop();
   server->TerminateConnections();
-  SPIN_WHILE(!server.closed);
+  SPIN_WHILE(!server.done());
+  SPIN_WHILE(uv_loop_alive(&loop));
 }
 
 TEST_F(InspectorSocketServerTest, ServerCannotStart) {
-  ServerDelegateNoTargets delegate1, delegate2;
-  ServerHolder server1(&delegate1, &loop, 0);
+  ServerHolder server1(false, &loop, 0);
   ASSERT_TRUE(server1->Start());
-  ServerHolder server2(&delegate2, &loop, server1.port());
+  ServerHolder server2(false, &loop, server1.port());
   ASSERT_FALSE(server2->Start());
-  server1->Stop(ServerHolder::CloseCallback);
+  server1->Stop();
   server1->TerminateConnections();
-  SPIN_WHILE(!server1.closed);
-  ASSERT_TRUE(delegate1.done);
+  SPIN_WHILE(!server1.done());
+  ASSERT_TRUE(server1.delegate_done);
+  SPIN_WHILE(uv_loop_alive(&loop));
 }
 
 TEST_F(InspectorSocketServerTest, StoppingServerDoesNotKillConnections) {
-  ServerDelegateNoTargets delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(false, &loop, 0);
   ASSERT_TRUE(server->Start());
   SocketWrapper socket1(&loop);
   socket1.Connect(HOST, server.port());
   socket1.TestHttpRequest("/json/list", "[ ]");
-  server->Stop(ServerHolder::CloseCallback);
-  SPIN_WHILE(!server.closed);
+  server->Stop();
   socket1.TestHttpRequest("/json/list", "[ ]");
   socket1.Close();
   uv_run(&loop, UV_RUN_DEFAULT);
-  ASSERT_TRUE(delegate.done);
+  ASSERT_TRUE(server.delegate_done);
 }
 
 TEST_F(InspectorSocketServerTest, ClosingConnectionReportsDone) {
-  ServerDelegateNoTargets delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(false, &loop, 0);
   ASSERT_TRUE(server->Start());
   SocketWrapper socket1(&loop);
   socket1.Connect(HOST, server.port());
   socket1.TestHttpRequest("/json/list", "[ ]");
-  server->Stop(ServerHolder::CloseCallback);
-  SPIN_WHILE(!server.closed);
+  server->Stop();
   socket1.TestHttpRequest("/json/list", "[ ]");
   socket1.Close();
   uv_run(&loop, UV_RUN_DEFAULT);
-  ASSERT_TRUE(delegate.done);
+  ASSERT_TRUE(server.delegate_done);
 }
 
 TEST_F(InspectorSocketServerTest, ClosingSocketReportsDone) {
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(true, &loop, 0);
   ASSERT_TRUE(server->Start());
   SocketWrapper socket1(&loop);
   socket1.Connect(HOST, server.port());
   socket1.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   socket1.Expect(WS_HANDSHAKE_RESPONSE);
-  server->Stop(ServerHolder::CloseCallback);
-  SPIN_WHILE(!server.closed);
-  ASSERT_FALSE(delegate.done);
+  server->Stop();
+  ASSERT_FALSE(server.delegate_done);
   socket1.Close();
-  SPIN_WHILE(!delegate.done);
+  SPIN_WHILE(!server.delegate_done);
 }
 
 TEST_F(InspectorSocketServerTest, TerminatingSessionReportsDone) {
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, 0);
+  ServerHolder server(true, &loop, 0);
   ASSERT_TRUE(server->Start());
   SocketWrapper socket1(&loop);
   socket1.Connect(HOST, server.port());
   socket1.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   socket1.Expect(WS_HANDSHAKE_RESPONSE);
-  server->Stop(ServerHolder::CloseCallback);
-  SPIN_WHILE(!server.closed);
-  ASSERT_FALSE(delegate.done);
+  server->Stop();
+  ASSERT_FALSE(server.delegate_done);
   server->TerminateConnections();
   socket1.Expect(SERVER_CLOSE_FRAME);
   socket1.Write(CLIENT_CLOSE_FRAME);
   socket1.ExpectEOF();
-  SPIN_WHILE(!delegate.done);
+  SPIN_WHILE(!server.delegate_done);
 }
 
 TEST_F(InspectorSocketServerTest, FailsToBindToNodejsHost) {
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, "nodejs.org", 80, nullptr);
+  ServerHolder server(true, &loop, "nodejs.org", 80, nullptr);
   ASSERT_FALSE(server->Start());
   SPIN_WHILE(uv_loop_alive(&loop));
 }
@@ -619,17 +595,14 @@ TEST_F(InspectorSocketServerTest, BindsToIpV6) {
     fprintf(stderr, "No IPv6 network detected\n");
     return;
   }
-  TestInspectorServerDelegate delegate;
-  ServerHolder server(&delegate, &loop, "::", 0, nullptr);
+  ServerHolder server(true, &loop, "::", 0, nullptr);
   ASSERT_TRUE(server->Start());
-
   SocketWrapper socket1(&loop);
   socket1.Connect("::1", server.port(), true);
   socket1.Write(WsHandshakeRequest(MAIN_TARGET_ID));
   socket1.Expect(WS_HANDSHAKE_RESPONSE);
-  server->Stop(ServerHolder::CloseCallback);
-  SPIN_WHILE(!server.closed);
-  ASSERT_FALSE(delegate.done);
+  server->Stop();
+  ASSERT_FALSE(server.delegate_done);
   socket1.Close();
-  SPIN_WHILE(!delegate.done);
+  SPIN_WHILE(!server.delegate_done);
 }