Debugger: cleanup server-side when the client disconnects while in run mode

Server-side cleanup was all driven by having a request thread see that either the connection with the client is closed, or the stopped flag on the proxy is set. However, if a debugger's in the run state and there's just no incoming requests against the right sandbox, then that would never happen. However, the signal polling thread is perfect to notice, there were just issues coordinating the cleanup of the proxy.

Modified stop() on the proxy to be callable from any thread, and to initiate cleanup of the proxy but pass the final cleanup off to another thread which can complete it. The signal polling and dummy sandbox threads are owned by the proxy, so they can't complete the work themselves. There was logic to queue cleanup just for the dummy sandbox, so I turned that into proxy cleanup and have it cleanup both.

I'll investigate making a unit test using the new framework that will test this, but that will come in a later diff since this diff is already quite involved.

hphp/doc/debugger.devdocs

@@ -261,6 +261,31 @@ destroy itself. The same actions will occur if the connection with the
 client is lost for any other reason, even if no CmdQuit was
 received. An error reading from the socket, a closed socket, etc.
 
+2.4.1 Cleaning the proxy
+------------------------
+
+There are many cases where the proxy will notice that a client has
+terminated the connection. The easiest one is when a quit command is
+received, but the client may exit for any number of reasons, and in
+any state. At a minimum, the proxy's signal polling thread will, after
+one second, notice that the connection has been dropped and initiate
+cleanup. However, neither the signal polling thread nor the dummy
+sandbox thread can completely perform the cleanup because they are
+owned by the proxy, and destroying the proxy would destroy those
+threads before they have completed.
+
+Thus, proxy cleanup may be initiated by any thread with Proxy::stop(),
+but the final cleanup is performed by another thread doing
+housekeeping work. The cleanup work waits for both the signal polling
+and dummy sandbox threads to exit before completing. Server-side this
+housekeeping work is done by the server thread, which is also
+listening for new debugger connections. Note that this cleanup work
+may complete while a request thread is still using the proxy. The last
+reference to the proxy will finally destroy it, and the cleanup work
+ensures that the proxy is still usable (and communicates that it is
+stopped) by any outstanding request threads.
+
+
 
 3.0 Client Implementation
 -------------------------

hphp/runtime/base/types.h

@@ -211,8 +211,7 @@ public:
   RequestInjectionData()
     : cflagsPtr(nullptr), surprisePage(nullptr),
       m_timeoutSeconds(-1), m_hasTimer(false), m_timerActive(false),
-      m_debugger(false), m_dummySandbox(false),
-      m_debuggerIntr(false), m_coverage(false),
+      m_debugger(false), m_debuggerIntr(false), m_coverage(false),
       m_jit(false) {
   }
 
@@ -238,7 +237,6 @@ public:
                          // Set true when we activate a timer,
                          // cleared when the signal handler runs
   bool m_debugger;       // whether there is a DebuggerProxy attached to me
-  bool m_dummySandbox;   // indicating it is from a dummy sandbox thread
   bool m_debuggerIntr;   // indicating we should force interrupt for debugger
   bool m_coverage;       // is coverage being collected
   bool m_jit;            // is the jit enabled
@@ -267,8 +265,6 @@ public:
     m_coverage = flag;
     updateJit();
   }
-  bool getDummySandbox() const { return m_dummySandbox; }
-  void setDummySandbox(bool ds) { m_dummySandbox = ds; }
   void updateJit();
 
   std::stack<void *> interrupts;   // CmdInterrupts this thread's handling

hphp/runtime/debugger/debugger.cpp

@@ -53,6 +53,7 @@ void Debugger::Stop() {
   LogShutdown(ShutdownKind::Normal);
   s_debugger.m_proxyMap.clear();
   DebuggerServer::Stop();
+  CleanupRetiredProxies();
   if (s_clientStarted) {
     DebuggerClient::Stop();
   }
@@ -118,14 +119,12 @@ void Debugger::RequestInterrupt(DebuggerProxyPtr proxy) {
   s_debugger.requestInterrupt(proxy);
 }
 
-void Debugger::RetireDummySandboxThread(DummySandbox* toRetire) {
-  TRACE(2, "Debugger::RetireDummySandboxThread\n");
-  s_debugger.retireDummySandboxThread(toRetire);
+void Debugger::RetireProxy(DebuggerProxyPtr proxy) {
+  s_debugger.retireProxy(proxy);
 }
 
-void Debugger::CleanupDummySandboxThreads() {
-  TRACE(7, "Debugger::CleanupDummySandboxThreads\n");
-  s_debugger.cleanupDummySandboxThreads();
+void Debugger::CleanupRetiredProxies() {
+  s_debugger.cleanupRetiredProxies();
 }
 
 void Debugger::DebuggerSession(const DebuggerClientOptions& options,
@@ -461,25 +460,35 @@ DebuggerProxyPtr Debugger::createProxy(SmartPtr<Socket> socket, bool local) {
   return proxy;
 }
 
-void Debugger::retireDummySandboxThread(DummySandbox* toRetire) {
-  TRACE(2, "Debugger::retireDummySandboxThread\n");
-  m_cleanupDummySandboxQ.push(toRetire);
+// Place the proxy onto the retired proxy queue. It will be cleaned up
+// and destroyed later by another thread.
+void Debugger::retireProxy(DebuggerProxyPtr proxy) {
+  TRACE(2, "Debugger::retireProxy %p\n", proxy.get());
+  m_retiredProxyQueue.push(proxy);
 }
 
-void Debugger::cleanupDummySandboxThreads() {
-  TRACE(7, "Debugger::cleanupDummySandboxThreads\n");
-  DummySandbox* ptr = nullptr;
-  while (m_cleanupDummySandboxQ.try_pop(ptr)) {
-    ptr->notify();
+// Cleanup any proxies in our retired proxy queue. We'll pull a proxy
+// out of the queue, ask it to cleanup which will wait for threads it
+// owns to exit, then drop our shared reference to it. That may
+// destroy the proxy here, or it may remain alive if there is a thread
+// still processing an interrupt with it since such threads have their
+// own shared reference.
+void Debugger::cleanupRetiredProxies() {
+  TRACE(7, "Debugger::cleanupRetiredProxies\n");
+  DebuggerProxyPtr proxy;
+  while (m_retiredProxyQueue.try_pop(proxy)) {
     try {
-      // we can't block the server for waiting
-      if (ptr->waitForEnd(1)) {
-        delete ptr;
-      } else {
-        Logger::Error("Dummy sandbox %p refused to stop", ptr);
+      // We give the proxy a short period of time to wait for any
+      // threads it owns. If it doesn't succeed, we put it back and
+      // try again later.
+      TRACE(2, "Cleanup proxy %p\n", proxy.get());
+      if (!proxy->cleanup(1)) {
+        TRACE(2, "Proxy %p has not stopped yet\n", proxy.get());
+        m_retiredProxyQueue.push(proxy);
       }
     } catch (Exception &e) {
-      Logger::Error("Dummy sandbox exception: " + e.getMessage());
+      Logger::Error("Exception during proxy %p retirement: %s",
+                    proxy.get(), e.getMessage().c_str());
     }
   }
 }

hphp/runtime/debugger/debugger.h

@@ -62,8 +62,8 @@ public:
   static void GetRegisteredSandboxes(DSandboxInfoPtrVec &sandboxes);
   static bool IsThreadDebugging(int64_t tid);
 
-  static void RetireDummySandboxThread(DummySandbox* toRetire);
-  static void CleanupDummySandboxThreads();
+  static void RetireProxy(DebuggerProxyPtr proxy);
+  static void CleanupRetiredProxies();
 
   // Request interrupt on threads that a proxy is attached to
   static void RequestInterrupt(DebuggerProxyPtr proxy);
@@ -149,8 +149,8 @@ private:
   typedef tbb::concurrent_hash_map<int64_t, ThreadInfo*> ThreadInfoMap;
   ThreadInfoMap m_threadInfos;
 
-  typedef tbb::concurrent_queue<DummySandbox*> DummySandboxQ;
-  DummySandboxQ m_cleanupDummySandboxQ;
+  typedef tbb::concurrent_queue<DebuggerProxyPtr> RetiredProxyQueue;
+  RetiredProxyQueue m_retiredProxyQueue;
 
   bool isThreadDebugging(int64_t id);
   void registerThread();
@@ -178,8 +178,8 @@ private:
                          bool force);
   bool switchSandbox(DebuggerProxyPtr proxy, const std::string &newId,
                      bool force);
-  void retireDummySandboxThread(DummySandbox* toRetire);
-  void cleanupDummySandboxThreads();
+  void retireProxy(DebuggerProxyPtr proxy);
+  void cleanupRetiredProxies();
 
   // A usage logger which is set by a provider to an implementation-specific
   // subclass if usage logging is enabled.

hphp/runtime/debugger/debugger_proxy.cpp

@@ -55,17 +55,64 @@ DebuggerProxy::DebuggerProxy(SmartPtr<Socket> socket, bool local)
   Debugger::UsageLog("server", m_dummyInfo.id(), "connect", clientDetails);
 }
 
-DebuggerProxy::~DebuggerProxy() {
-  TRACE_RB(2, "DebuggerProxy::~DebuggerProxy starting\n");
-  stop();
-  m_signalThread.waitForEnd();
-
+// Cleanup all resources owned by this proxy, including any threads it
+// owns. If a thread doesn't stop, return false so we can try again
+// later. This can be called multiple times, and this function leaves
+// the proxy usable by request threads which may still be handling an
+// interrupt.
+bool DebuggerProxy::cleanup(int timeout) {
+  TRACE_RB(2, "DebuggerProxy::cleanup starting\n");
+  // If we're not already marked as stopping then there may be other
+  // threads still attempting to use this object!
+  assert(m_stopped);
+  // No more client operation is possible, so drop the connection.
+  m_thrift.close();
+  TRACE(2, "Stopping signal thread...\n");
+  if (!m_signalThread.waitForEnd(timeout)) return false;
+  TRACE(2, "Stopping dummy sandbox...\n");
   if (m_dummySandbox) {
-    m_dummySandbox->stop();
+    if (!m_dummySandbox->stop(timeout)) return false;
+    delete m_dummySandbox;
+    m_dummySandbox = nullptr;
+  }
+  TRACE_RB(2, "DebuggerProxy::cleanup complete\n");
+  return true;
+}
+
+// Stop the proxy and ensure that no new uses of it can
+// occur. Schedule it for final cleanup on another thread. This may be
+// called from any thread wishing to stop the proxy, for any reason.
+void DebuggerProxy::stop() {
+  // Shared ref to keep us alive. Often the only reference to a proxy
+  // is in the debugger's proxy map, which we're about to alter below.
+  auto this_ = shared_from_this();
+  Debugger::UsageLog("server", getSandboxId(), "disconnect");
+
+  // After this we'll no longer be able to get this_ from
+  // Debugger::findProxy(), which means no new interrupts for this
+  // proxy. There may still be threads in existing interrupts, though.
+  Debugger::RemoveProxy(this_);
+
+  // Pop the signal polling thread, and any interrupting threads, out
+  // of their event loops and cause them to exit.
+  DSandboxInfo invalid;
+  {
+    Lock lock(this);
+    m_sandbox = invalid;
+    m_stopped = true;
   }
 
-  Debugger::UsageLog("server", getSandboxId(), "disconnect");
-  TRACE_RB(2, "DebuggerProxy::~DebuggerProxy complete\n");
+  // Retire the proxy, which will schedule it for cleanup and
+  // destruction on another thread. Threads for the dummy sandbox and
+  // signal polling can't destroy the proxy directly, since the proxy
+  // owns them and will want to destroy them.
+  Debugger::RetireProxy(this_);
+}
+
+// Stop the proxy, and stop execution of the current request.
+void DebuggerProxy::stopAndThrow() {
+  stop();
+  throw DebuggerClientExitException();
 }
 
 const char *DebuggerProxy::getThreadType() const {
@@ -73,15 +120,15 @@ const char *DebuggerProxy::getThreadType() const {
   return isLocal() ? "Command Line Script" : "Dummy Sandbox";
 }
 
-DSandboxInfo DebuggerProxy::getSandbox() const {
+DSandboxInfo DebuggerProxy::getSandbox() {
   TRACE(2, "DebuggerProxy::getSandbox\n");
-  Lock lock(m_mutex);
+  Lock lock(this);
   return m_sandbox;
 }
 
-std::string DebuggerProxy::getSandboxId() const {
+std::string DebuggerProxy::getSandboxId() {
   TRACE(2, "DebuggerProxy::getSandboxId\n");
-  Lock lock(m_mutex);
+  Lock lock(this);
   return m_sandbox.id();
 }
 
@@ -117,7 +164,7 @@ bool DebuggerProxy::switchSandbox(const std::string &newId, bool force) {
 // map.
 void DebuggerProxy::updateSandbox(DSandboxInfoPtr sandbox) {
   TRACE(2, "DebuggerProxy::updateSandbox\n");
-  Lock lock(m_mutex);
+  Lock lock(this);
   if (sandbox) {
     if (m_sandbox.id() != sandbox->id()) {
       m_sandbox = *sandbox;
@@ -167,7 +214,7 @@ void DebuggerProxy::startDummySandbox() {
 
 void DebuggerProxy::notifyDummySandbox() {
   TRACE(2, "DebuggerProxy::notifyDummySandbox\n");
-  m_dummySandbox->notifySignal(CmdSignal::SignalBreak);
+  if (m_dummySandbox) m_dummySandbox->notifySignal(CmdSignal::SignalBreak);
 }
 
 void DebuggerProxy::setBreakPoints(BreakPointInfoPtrVec &breakpoints) {
@@ -310,20 +357,19 @@ void DebuggerProxy::pollSignal() {
   while (!m_stopped) {
     sleep(1);
 
-    // After DebuggerSignalTimeout seconds that no active thread picks
-    // up the signal, we send it to dummy sandbox.
-    if ((m_signum != CmdSignal::SignalNone) && m_dummySandbox) {
-      Lock lock(m_signumMutex);
-      if ((m_signum != CmdSignal::SignalNone) && (--signalTimeout <= 0)) {
-        TRACE_RB(2, "DebuggerProxy::pollSignal: sending to dummy sandbox\n");
-        m_dummySandbox->notifySignal(m_signum);
-        m_signum = CmdSignal::SignalNone;
-      }
-    }
-
     // Block any threads that might be interrupting from communicating with the
     // client until we're done with this poll.
-    Lock lock(m_signalMutex);
+    Lock lock(m_signumMutex);
+
+    // After DebuggerSignalTimeout seconds that no active thread picks
+    // up the signal, we send it to dummy sandbox.
+    if ((m_signum != CmdSignal::SignalNone) && m_dummySandbox &&
+        (--signalTimeout <= 0)) {
+      TRACE_RB(2, "DebuggerProxy::pollSignal: sending to dummy sandbox\n");
+      m_dummySandbox->notifySignal(m_signum);
+      m_signum = CmdSignal::SignalNone;
+    }
+
     // Don't actually poll if another thread is already in a command
     // processing loop with the client.
     if (!m_okayToPoll) continue;
@@ -336,11 +382,13 @@ void DebuggerProxy::pollSignal() {
       break;
     }
 
-    // We will loop forever until DebuggerClient sends us something, modulo
-    // transport failure or a shutdown request.
+    // We've sent the client a command, and we expect an immediate
+    // response. Wait 10 times to give it a chance on especially
+    // overloaded computers.
     DebuggerCommandPtr res;
-    while (!DebuggerCommand::Receive(m_thrift, res,
-                                     "DebuggerProxy::pollSignal()")) {
+    for (int i = 0; i < 10; i++) {
+      if (DebuggerCommand::Receive(m_thrift, res,
+                                   "DebuggerProxy::pollSignal()")) break;
       if (m_stopped) {
         TRACE_RB(2, "DebuggerProxy::pollSignal: "
                  "signal thread asked to stop while waiting "
@@ -374,9 +422,7 @@ void DebuggerProxy::pollSignal() {
     }
   }
   if (!m_stopped) {
-    // We've noticed that the socket has closed. If there is a thread stopped at
-    // an interrrupt, stop() will help pop it out and cause the proxy to throw
-    // the proper exception to terminate the request.
+    // We've noticed that the socket has closed. Stop and destory this proxy.
     TRACE_RB(2, "DebuggerProxy::pollSignal: "
              "lost communication with the client, stopping proxy\n");
     stop();
@@ -384,31 +430,6 @@ void DebuggerProxy::pollSignal() {
   TRACE_RB(2, "DebuggerProxy::pollSignal: ended\n");
 }
 
-// Ask this proxy to stop running and exit cleanly. Used during proxy
-// cleanup, sandbox switching, and from the signal polling
-// thread. This can be used from a thread which is not stopped at the
-// given proxy's interrupt.
-void DebuggerProxy::stop() {
-  TRACE_RB(2, "DebuggerProxy::stop\n");
-  DSandboxInfo invalid;
-  Lock l(this);
-  m_sandbox = invalid;
-  m_stopped = true;
-  // the flag will take care of the rest
-}
-
-// Used to quit this proxy, typcially in response to either a quit command from
-// the client or loss of communication with the client. The proxy is removed
-// from the proxy map, ensuring no other threads can use the proxy.
-// NB: It stops the proxy, and then tosses the client exit exception
-// to ensure the current request is terminated with a nice message.
-void DebuggerProxy::forceQuit() {
-  TRACE_RB(2, "DebuggerProxy::forceQuit\n");
-  Debugger::RemoveProxy(shared_from_this());
-  stop();
-  throw DebuggerClientExitException();
-}
-
 // Grab the ip address and port of the client that is connected to this proxy.
 bool DebuggerProxy::getClientConnectionInfo(VRefParam address,
                                             VRefParam port) {
@@ -584,7 +605,7 @@ bool DebuggerProxy::checkFlowBreak(CmdInterrupt &cmd) {
 
 void DebuggerProxy::checkStop() {
   TRACE(5, "DebuggerProxy::checkStop\n");
-  if (m_stopped) forceQuit();
+  if (m_stopped) stopAndThrow();
 }
 
 void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
@@ -592,8 +613,7 @@ void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
   // Do the server-side work for this interrupt, which just notifies the client.
   if (!cmd.onServer(*this)) {
     TRACE_RB(1, "Failed to send CmdInterrupt to client\n");
-    Debugger::RemoveProxy(shared_from_this()); // on socket error
-    return;
+    stopAndThrow();
   }
 
   Debugger::UsageLogInterrupt("server", getSandboxId(), cmd);
@@ -633,7 +653,7 @@ void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
       if (res->is(DebuggerCommand::KindOfQuit)) {
         TRACE_RB(2, "Received quit command\n");
         res->onServer(*this); // acknowledge receipt so that client can quit.
-        forceQuit();
+        stopAndThrow();
       }
     }
     bool cmdFailure = false;
@@ -655,7 +675,7 @@ void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
                res->getType());
       cmdFailure = true;
     }
-    if (cmdFailure) forceQuit();
+    if (cmdFailure) stopAndThrow();
     if (res->shouldExitInterrupt()) return;
   }
 }

hphp/runtime/debugger/debugger_proxy.h

@@ -61,13 +61,12 @@ public:
 
 public:
   DebuggerProxy(SmartPtr<Socket> socket, bool local);
-  ~DebuggerProxy();
 
   bool isLocal() const { return m_local;}
 
   const char *getThreadType() const;
-  DSandboxInfo getSandbox() const;
-  std::string getSandboxId() const;
+  DSandboxInfo getSandbox();
+  std::string getSandboxId();
   const DSandboxInfo& getDummyInfo() const { return m_dummyInfo; }
 
   void getThreads(DThreadInfoPtrVec &threads);
@@ -95,11 +94,8 @@ public:
   int getRealStackDepth();
 
   void startSignalThread();
-  void pollSignal(); // for signal polling thread
-
-  void checkStop();
-  void forceQuit();
   void stop();
+  bool cleanup(int timeout);
 
   bool getClientConnectionInfo(VRefParam address, VRefParam port);
 
@@ -119,6 +115,9 @@ private:
   void processInterrupt(CmdInterrupt &cmd);
   void enableSignalPolling();
   void disableSignalPolling();
+  void checkStop();
+  void pollSignal(); // for signal polling thread
+  void stopAndThrow();
 
   DThreadInfoPtr createThreadInfo(const std::string &desc);
 
@@ -130,7 +129,6 @@ private:
   DebuggerThriftBuffer m_thrift;
   DummySandbox* m_dummySandbox;
 
-  mutable Mutex m_mutex;
   ReadWriteMutex m_breakMutex;
   bool m_hasBreakPoints;
   BreakPointInfoPtrVec m_breakpoints;

hphp/runtime/debugger/debugger_server.cpp

@@ -143,8 +143,9 @@ void DebuggerServer::accept() {
         }
       }
     } // else timed out, then we have a chance to check m_stopped bit
-    // cleanup the dummy sandbox threads it created
-    Debugger::CleanupDummySandboxThreads();
+
+    // A chance for some housekeeping...
+    Debugger::CleanupRetiredProxies();
   }
 
   m_sock.reset();

hphp/runtime/debugger/dummy_sandbox.cpp

@@ -35,40 +35,21 @@ DummySandbox::DummySandbox(DebuggerProxy *proxy,
                            const std::string &defaultPath,
                            const std::string &startupFile)
     : m_proxy(proxy), m_defaultPath(defaultPath), m_startupFile(startupFile),
-      m_stopped(false),
-      m_signum(CmdSignal::SignalNone) {
-  TRACE(2, "DummySandbox::DummySandbox\n");
-  m_thread = new AsyncFunc<DummySandbox>(this, &DummySandbox::run);
-}
-
-bool DummySandbox::waitForEnd(int seconds) {
-  TRACE(2, "DummySandbox::waitForEnd\n");
-  bool ret = m_thread->waitForEnd(seconds);
-  if (ret) {
-    delete m_thread;
-  }
-  return ret;
-}
+      m_thread(this, &DummySandbox::run), m_stopped(false),
+      m_signum(CmdSignal::SignalNone) { }
 
 void DummySandbox::start() {
   TRACE(2, "DummySandbox::start\n");
-  m_thread->start();
+  m_thread.start();
 }
 
-void DummySandbox::stop() {
+// Stop the sandbox thread, and wait for it to end. Timeout is in
+// seconds. This can be called multiple times.
+bool DummySandbox::stop(int timeout) {
   TRACE(2, "DummySandbox::stop\n");
   m_stopped = true;
-  ThreadInfo *ti = ThreadInfo::s_threadInfo.getNoCheck();
-  if (ti->m_reqInjectionData.getDummySandbox()) {
-    // called from dummy sandbox thread itself, schedule retirement
-    Debugger::RetireDummySandboxThread(this);
-  } else {
-    // called from worker thread, we wait for the dummySandbox to end
-    m_thread->waitForEnd();
-    // we are sure it's always created by new and this is the last thing
-    // on this object
-    delete this;
-  }
+  notify(); // Wakeup the sandbox thread so it will notice the stopped flag
+  return m_thread.waitForEnd(timeout);
 }
 
 namespace {
@@ -96,7 +77,6 @@ void DummySandbox::run() {
   TRACE(2, "DummySandbox::run\n");
   ThreadInfo *ti = ThreadInfo::s_threadInfo.getNoCheck();
   Debugger::RegisterThread();
-  ti->m_reqInjectionData.setDummySandbox(true);
   while (!m_stopped) {
     try {
       CLISession hphpSession;

hphp/runtime/debugger/dummy_sandbox.h

@@ -34,9 +34,8 @@ class DummySandbox : public Synchronizable {
 public:
   DummySandbox(DebuggerProxy *proxy, const std::string &defaultPath,
                const std::string &startupFile);
-  bool waitForEnd(int seconds);
   void start();
-  void stop();
+  bool stop(int timeout);
 
   // execution thread
   void run();
@@ -48,7 +47,7 @@ private:
   std::string m_defaultPath;
   std::string m_startupFile;
 
-  AsyncFunc<DummySandbox>* m_thread;
+  AsyncFunc<DummySandbox> m_thread;
   bool m_stopped;
   int m_signum;