inspector: code cleanup

Remove some unused code from the WS server implementation and switch to smart pointers where possible. PR-URL: #21070 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Jun 5, 2018 · 327ce2d · 327ce2d
1 parent 0300f7c
commit 327ce2d
Show file tree

Hide file tree

Showing 7 changed files with 229 additions and 337 deletions.
diff --git 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,14 +415,15 @@ bool InspectorIo::WaitForFrontendEvent() {
 }
 
 InspectorIoDelegate::InspectorIoDelegate(InspectorIo* io,
+                                         const std::string& target_id,
                                          const std::string& script_path,
                                          const std::string& script_name,
                                          bool wait)
                                          : io_(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
@@ -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
@@ -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,12 +717,13 @@ 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) {

diff --git 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);
 };