Avoid stalls when MainLoop::Interrupt fails to wake up the MainLoop (#164905)

Turns out there's a bug in the current lldb sources that if you fork,
set the stdio file handles to close on exec and then exec lldb with some
commands and the `--batch` flag, lldb will stall on exit. The first
cause of the bug is that the Python session handler - and probably other
places in lldb - think 0, 1, and 2 HAVE TO BE the stdio file handles,
and open and close and dup them as needed. NB: I am NOT trying to fix
that bug. I'm not convinced running the lldb driver headless is worth a
lot of effort, it's just as easy to redirect them to /dev/null, which
does work.

But I would like to keep lldb from stalling on the way out when this
happens. The reason we stall is that we have a MainLoop waiting for
signals, and we try to Interrupt it, but because stdio was closed, the
interrupt pipe for the MainLoop gets the file descriptor 0, which gets
closed by the Python session handler if you run some script command. So
the Interrupt fails.

We were running the Write to the interrupt pipe wrapped in
`llvm::cantFail`, but in a no asserts build that just drops the error on
the floor. So then lldb went on to call std::thread::join on the still
active MainLoop, and that stalls

I made Interrupt (and AddCallback & AddPendingCallback) return a bool
for "interrupt success" instead. All the places where code was
requesting termination, I added checks for that failure, and skip the
std::thread::join call on the MainLoop thread, since that is almost
certainly going to stall at this point.

I didn't do the same for the Windows MainLoop, as I don't know if/when
the WSASetEvent call can fail, so I always return true here. I also
didn't turn the test off for Windows. According to the Python docs all
the API's I used should work on Windows... If that turns out not to be
true I'll make the test Darwin/Unix only.

Author: jimingham

lldb/include/lldb/Host/MainLoopBase.h

@@ -57,18 +57,23 @@ class MainLoopBase {
   // Add a pending callback that will be executed once after all the pending
   // events are processed. The callback will be executed even if termination
   // was requested.
-  void AddPendingCallback(const Callback &callback) {
-    AddCallback(callback, std::chrono::steady_clock::time_point());
+  // Returns false if an interrupt was needed to get the loop to act on the new
+  // callback, but the interrupt failed, true otherwise.  Mostly used when the
+  // pending callback is a RequestTermination, since if the interrupt fails for
+  // that callback, waiting for the MainLoop thread to terminate could stall.
+  bool AddPendingCallback(const Callback &callback) {
+    return AddCallback(callback, std::chrono::steady_clock::time_point());
   }
 
   // Add a callback that will be executed after a certain amount of time has
-  // passed.
-  void AddCallback(const Callback &callback, std::chrono::nanoseconds delay) {
-    AddCallback(callback, std::chrono::steady_clock::now() + delay);
+  // passed.  See AddPendingCallback comment for the return value.
+  bool AddCallback(const Callback &callback, std::chrono::nanoseconds delay) {
+    return AddCallback(callback, std::chrono::steady_clock::now() + delay);
   }
 
   // Add a callback that will be executed after a given point in time.
-  void AddCallback(const Callback &callback, TimePoint point);
+  // See AddPendingCallback comment for the return value.
+  bool AddCallback(const Callback &callback, TimePoint point);
 
   // Waits for registered events and invoke the proper callbacks. Returns when
   // all callbacks deregister themselves or when someone requests termination.
@@ -85,8 +90,9 @@ class MainLoopBase {
 
   virtual void UnregisterReadObject(IOObject::WaitableHandle handle) = 0;
 
-  // Interrupt the loop that is currently waiting for events.
-  virtual void Interrupt() = 0;
+  /// Interrupt the loop that is currently waiting for events.  Return true if
+  /// the interrupt succeeded, false if it failed.
+  virtual bool Interrupt() = 0;
 
   void ProcessCallbacks();
 

lldb/include/lldb/Host/posix/MainLoopPosix.h

@@ -54,7 +54,7 @@ class MainLoopPosix : public MainLoopBase {
   void UnregisterReadObject(IOObject::WaitableHandle handle) override;
   void UnregisterSignal(int signo, std::list<Callback>::iterator callback_it);
 
-  void Interrupt() override;
+  bool Interrupt() override;
 
 private:
   void ProcessReadObject(IOObject::WaitableHandle handle);

lldb/include/lldb/Host/windows/MainLoopWindows.h

@@ -50,7 +50,7 @@ class MainLoopWindows : public MainLoopBase {
 protected:
   void UnregisterReadObject(IOObject::WaitableHandle handle) override;
 
-  void Interrupt() override;
+  bool Interrupt() override;
 
 private:
   llvm::Expected<size_t> Poll();

lldb/source/Host/common/MainLoopBase.cpp

@@ -12,8 +12,9 @@
 using namespace lldb;
 using namespace lldb_private;
 
-void MainLoopBase::AddCallback(const Callback &callback, TimePoint point) {
+bool MainLoopBase::AddCallback(const Callback &callback, TimePoint point) {
   bool interrupt_needed;
+  bool interrupt_succeeded = true;
   {
     std::lock_guard<std::mutex> lock{m_callback_mutex};
     // We need to interrupt the main thread if this callback is scheduled to
@@ -22,7 +23,8 @@ void MainLoopBase::AddCallback(const Callback &callback, TimePoint point) {
     m_callbacks.emplace(point, callback);
   }
   if (interrupt_needed)
-    Interrupt();
+    interrupt_succeeded = Interrupt();
+  return interrupt_succeeded;
 }
 
 void MainLoopBase::ProcessCallbacks() {

lldb/source/Host/posix/MainLoopPosix.cpp

@@ -387,10 +387,11 @@ void MainLoopPosix::ProcessSignal(int signo) {
   }
 }
 
-void MainLoopPosix::Interrupt() {
+bool MainLoopPosix::Interrupt() {
   if (m_interrupting.exchange(true))
-    return;
+    return true;
 
   char c = '.';
-  cantFail(m_interrupt_pipe.Write(&c, 1));
+  llvm::Expected<size_t> result = m_interrupt_pipe.Write(&c, 1);
+  return result && *result != 0;
 }

lldb/source/Host/windows/MainLoopWindows.cpp

@@ -272,4 +272,6 @@ Status MainLoopWindows::Run() {
   return Status();
 }
 
-void MainLoopWindows::Interrupt() { WSASetEvent(m_interrupt_event); }
+bool MainLoopWindows::Interrupt() {
+  return WSASetEvent(m_interrupt_event);
+}

lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp

@@ -133,11 +133,12 @@ llvm::Error ProtocolServerMCP::Stop() {
   }
 
   // Stop the main loop.
-  m_loop.AddPendingCallback(
+  bool addition_succeeded = m_loop.AddPendingCallback(
       [](lldb_private::MainLoopBase &loop) { loop.RequestTermination(); });
 
-  // Wait for the main loop to exit.
-  if (m_loop_thread.joinable())
+  // Wait for the main loop to exit, but not if we didn't succeed in inserting
+  // our pending callback or we'll wait forever.
+  if (addition_succeeded && m_loop_thread.joinable())
     m_loop_thread.join();
 
   m_accept_handles.clear();

lldb/test/API/driver/stdio_closed/TestDriverWithClosedSTDIO.py

@@ -0,0 +1,51 @@
+"""
+Test that if you exec lldb with the stdio file handles
+closed, it is able to exit without hanging.
+"""
+
+
+import lldb
+import os
+import sys
+import socket
+import fcntl
+
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestDriverWithClosedSTDIO(TestBase):
+    # If your test case doesn't stress debug info, then
+    # set this to true.  That way it won't be run once for
+    # each debug info format.
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_run_lldb_and_wait(self):
+        """This test forks, closes the stdio channels and exec's lldb.
+        Then it waits for it to exit and asserts it did that successfully"""
+        pid = os.fork()
+        if pid == 0:
+            fcntl.fcntl(sys.stdin, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
+            fcntl.fcntl(sys.stdout, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
+            fcntl.fcntl(sys.stderr, fcntl.F_SETFD, fcntl.FD_CLOEXEC)
+            lldb = lldbtest_config.lldbExec
+            print(f"About to run: {lldb}")
+            os.execlp(
+                lldb,
+                lldb,
+                "-x",
+                "-o",
+                "script print(lldb.debugger.GetNumTargets())",
+                "--batch",
+            )
+        else:
+            if pid == -1:
+                print("Couldn't fork a process.")
+                return
+            ret_pid, status = os.waitpid(pid, 0)
+            # We're really just checking that lldb doesn't stall.
+            # At the time this test was written, if you close stdin
+            # in an asserts build, lldb aborts.  So handle both
+            # of those cases.  The failure will just be that the
+            # waitpid doesn't return, and the test times out.
+            self.assertFalse(os.WIFSTOPPED(status), "We either exited or crashed.")

lldb/tools/driver/Driver.cpp

@@ -902,9 +902,10 @@ int main(int argc, char const *argv[]) {
   }
 
 #if !defined(_WIN32)
-  signal_loop.AddPendingCallback(
-      [](MainLoopBase &loop) { loop.RequestTermination(); });
-  signal_thread.join();
+  // Try to interrupt the signal thread.  If that succeeds, wait for it to exit.
+  if (signal_loop.AddPendingCallback(
+          [](MainLoopBase &loop) { loop.RequestTermination(); }))
+    signal_thread.join();
 #endif
 
   return exit_code;

lldb/tools/lldb-dap/DAP.cpp

@@ -1104,9 +1104,11 @@ llvm::Error DAP::Loop() {
                                      "unhandled packet");
   }
 
-  m_loop.AddPendingCallback(
-      [](MainLoopBase &loop) { loop.RequestTermination(); });
-  thread.join();
+  // Don't wait to join the mainloop thread if our callback wasn't added
+  // successfully, or we'll wait forever.
+  if (m_loop.AddPendingCallback(
+          [](MainLoopBase &loop) { loop.RequestTermination(); }))
+    thread.join();
 
   if (m_error_occurred)
     return llvm::createStringError(llvm::inconvertibleErrorCode(),