HHVM Debugger: Fix hang in debugger extension shutdown

ebluestein · fredemmott · commit a8fd6b8d1c47 · 2018-08-10T13:41:11.000-07:00
Summary:
This fixes a race preventing the debugger extension from shutting down cleanly.

The session cleanup thread deletes sessions in an async job because the session thread can be blocked running user code (its the HPHP request backing the console REPL). It therefore can block indefinitely if the user has called into a native method or has an infinite loop etc, and so we can't wait to join to it on the thread that needs to communicate with the debugger client.

If this thread is cleaning up sessions due to the client disconnecting, and the extension shutdown thread signals m_sessionCleanupCondition before the cleanup thread re-acquires the lock, we can erroneously go to sleep again because we're failing to re-check m_sessionCleanupTerminating after re-acquiring the lock but before sleeping on the condition variable.

Reviewed By: alexeyt

Differential Revision: D9038429

fbshipit-source-id: 67fe04876b0f4845cb5ff9418d301cf192ddcc75
diff --git a/hphp/runtime/ext/vsdebug/debugger.cpp b/hphp/runtime/ext/vsdebug/debugger.cpp
@@ -46,10 +46,18 @@ void Debugger::runSessionCleanupThread() {
   while (true) {
     {
       std::unique_lock<std::mutex> lock(m_sessionCleanupLock);
-      m_sessionCleanupCondition.wait(lock);
 
+      // Read m_sessionCleanupTerminating while the lock is held.
       terminating = m_sessionCleanupTerminating;
 
+      if (!terminating) {
+        // Wait for signal
+        m_sessionCleanupCondition.wait(lock);
+
+        // Re-check terminating flag while the lock is still re-acquired.
+        terminating = m_sessionCleanupTerminating;
+      }
+
       // Make a local copy of the session pointers to delete and drop
       // the lock.
       sessionsToDelete = m_cleanupSessions;
