[lldb] Ensure MainLoop only triggers once on EOF with kqueue.

[ashgti]

With kqueue backed MainLoop's, if the write side of the file is closed and EOF is reached we will continously trigger the read handle callback. This effectively turns the MainLoop into a busy loop.

To reproduce this, you can open a pipe, register the read end with a MainLoop and then close the write end.

To mitigate this, I added logic to remove FDs from the kqueue set once we detect the file has reached the end of the file.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

With kqueue backed MainLoop's, if the write side of the file is closed and EOF is reached we will continously trigger the read handle callback. This effectively turns the MainLoop into a busy loop.

To reproduce this, you can open a pipe, register the read end with a MainLoop and then close the write end.

To mitigate this, I added logic to remove FDs from the kqueue set once we detect the file has reached the end of the file.

---

Full diff: https://github.com/llvm/llvm-project/pull/179513.diff

2 Files Affected:

• (modified) lldb/source/Host/posix/MainLoopPosix.cpp (+15-2)
• (modified) lldb/unittests/Host/MainLoopTest.cpp (+5-2)

diff --git a/lldb/source/Host/posix/MainLoopPosix.cpp b/lldb/source/Host/posix/MainLoopPosix.cpp
index c6fe7814bd22e..2d95100ffe7e6 100644
--- a/lldb/source/Host/posix/MainLoopPosix.cpp
+++ b/lldb/source/Host/posix/MainLoopPosix.cpp
@@ -107,6 +107,11 @@ class MainLoopPosix::RunImpl {
 
 #if HAVE_SYS_EVENT_H
   std::vector<struct kevent> in_events;
+  enum FDState : bool {
+    eFDStateReading = false,
+    eFDStateEOF = true,
+  };
+  std::map<int, FDState> eof_fds;
   struct kevent out_events[4];
   int num_events = -1;
 
@@ -123,8 +128,12 @@ MainLoopPosix::RunImpl::RunImpl(MainLoopPosix &loop) : loop(loop) {
 Status MainLoopPosix::RunImpl::Poll() {
   in_events.resize(loop.m_read_fds.size());
   unsigned i = 0;
-  for (auto &fd : loop.m_read_fds)
-    EV_SET(&in_events[i++], fd.first, EVFILT_READ, EV_ADD, 0, 0, 0);
+  for (auto &fd : loop.m_read_fds) {
+    if (eof_fds.find(fd.first) == eof_fds.end())
+      eof_fds[fd.first] = eFDStateReading;
+    EV_SET(&in_events[i++], fd.first, EVFILT_READ,
+           eof_fds[fd.first] == eFDStateEOF ? EV_DELETE : EV_ADD, 0, 0, 0);
+  }
 
   num_events =
       kevent(loop.m_kqueue, in_events.data(), in_events.size(), out_events,
@@ -149,6 +158,10 @@ void MainLoopPosix::RunImpl::ProcessReadEvents() {
     switch (out_events[i].filter) {
     case EVFILT_READ:
       loop.ProcessReadObject(out_events[i].ident);
+      // A read event with `data == 0` indicates the file has reached EOF and is
+      // closed. Remember this to prevent triggering the callback again.
+      if (out_events[i].data == 0)
+        eof_fds[out_events[i].ident] = eFDStateEOF;
       break;
     default:
       llvm_unreachable("Unknown event");
diff --git a/lldb/unittests/Host/MainLoopTest.cpp b/lldb/unittests/Host/MainLoopTest.cpp
index 8a248100c936a..ebef25c5f8588 100644
--- a/lldb/unittests/Host/MainLoopTest.cpp
+++ b/lldb/unittests/Host/MainLoopTest.cpp
@@ -501,12 +501,15 @@ TEST_F(MainLoopTest, DetectsEOF) {
 
   Status error;
   MainLoop loop;
-  auto handle =
-      loop.RegisterReadObject(conn->GetReadObject(), make_callback(), error);
+  auto handle = loop.RegisterReadObject(
+      conn->GetReadObject(), [&](auto &loop) { callback_count++; }, error);
+  loop.AddCallback([](MainLoopBase &loop) { loop.RequestTermination(); },
+                   std::chrono::milliseconds(100));
   ASSERT_TRUE(error.Success());
   term.CloseSecondaryFileDescriptor();
 
   ASSERT_TRUE(loop.Run().Success());
+  // Ensure we only receive one event for the EOF event.
   ASSERT_EQ(1u, callback_count);
 }
 

[github-actions]

🐧 Linux x64 Test Results

• 33297 tests passed
• 507 tests skipped
• 1 test failed

Failed Tests

(click on a test name to see its output)

lldb-unit

lldb-unit.Host/_/HostTests/MainLoopTest/DetectsEOF

Script:
--
/home/gha/actions-runner/_work/llvm-project/llvm-project/build/tools/lldb/unittests/Host/./HostTests --gtest_filter=MainLoopTest.DetectsEOF
--
/home/gha/actions-runner/_work/llvm-project/llvm-project/lldb/unittests/Host/MainLoopTest.cpp:559
Expected equality of these values:
  1u
    Which is: 1
  callback_count
    Which is: 135829

If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the infrastructure label.

[bulbazord]

Is there a reason to keep the reader sides after reaching EOF? That is to say, why not remove them from loop.m_read_fds instead of keeping additional metadata?

I'm not too familiar with the specifics of kqueue

[ashgti]

Converting to a draft, this is also triggering on linux with poll and this may not be the correct solution to this issue.

I am looking into this further to understand what the right solution would be.

Is there a reason to keep the reader sides after reaching EOF? That is to say, why not remove them from loop.m_read_fds instead of keeping additional metadata?

I think UnregisterReadObject is currently only expecting a single call for a handle. I could refactor that to check if the handle has been unregistered already and we could invoke that directly, however I wasn't sure if that was the expected behavior for the handle to become unregistered during a 'Run' call.

[ashgti]

I don't think this can be addressed at the MainLoop level, we don't know enough information to determine if we've reached the EOF because there is no more data (e.g. a socket or pipe that has consumed all the current data but is NOT closed) vs reached the EOF of a file.

Instead, I am addressing the underlying issue in the JSONTransport layer in #179564