[lldb-dap] Make the DAP server resilient against broken pipes

[JDevlieghere]

This makes the DAP test server resilient against broken pipes. I'm not sure what is causing the broken pipe, but IIRC this isn't the first time we've seen an issues related to that. I worry about sweeping it under the rug, but on the other hand having the test suite hangup isn't great either.

Based this on https://bugs.python.org/issue21619:

The best way to clean up a subprocess that I have come up with to
close the pipe(s) and call wait() in two separate steps, such as:

try:
    proc.stdin.close()
except BrokenPipeError:
    pass
proc.wait()

Fixes #133782 (or rather: works around it)

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

This makes the DAP test server resilient against broken pipes. I'm not sure what is causing the broken pipe, but IIRC this isn't the first time we've seen an issues related to that. I worry about sweeping it under the rug, but on the other hand having the test suite hangup isn't great either.

Based this on https://bugs.python.org/issue21619:

> The best way to clean up a subprocess that I have come up with to
> close the pipe(s) and call wait() in two separate steps, such as:

try:
    proc.stdin.close()
except BrokenPipeError:
    pass
proc.wait()

Fixes #133782 (or rather: works around it)

---

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

1 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+4-2)

diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 01ef4b68f2653..57907b1d2f19b 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -1174,8 +1174,10 @@ def request_testGetTargetBreakpoints(self):
         return self.send_recv(command_dict)
 
     def terminate(self):
-        self.send.close()
-        # self.recv.close()
+        try:
+            self.send.close()
+        except BrokenPipeError:
+            pass
 
     def request_setInstructionBreakpoints(self, memory_reference=[]):
         breakpoints = []

[ashgti]

A SIGPIPE is raised if your process writes a pipe that is closed on the other end. If the python script is getting that then lldb-dap must have closed stdin.

The Transport input does take ownership of stdin (https://github.com/llvm/llvm-project/blob/main/lldb/tools/lldb-dap/lldb-dap.cpp#L574) so that will be closed once Transport is dealloced, but I don't think that would happen unless main had already exited.

I think its probably fine to catch this, but the SIGPIPE can also happen if you try to write to the FD as well.

[JDevlieghere]

This doesn't seem to be enough. When I added this, the SIGHUP disappeared for a bunch of runs, but now I'm seeing it again, with this patch applied.

[ashgti]

Does setting the 3rd param to false on

llvm-project/lldb/tools/lldb-dap/lldb-dap.cpp

Line 574 in 5e2860a

fileno(stdin), File::eOpenOptionReadOnly, true);

change anything for you?

[JDevlieghere]

Does setting the 3rd param to false on

llvm-project/lldb/tools/lldb-dap/lldb-dap.cpp

Line 574 in 5e2860a

fileno(stdin), File::eOpenOptionReadOnly, true);

change anything for you?

No, I tried that but I'm still getting the SIGHUP.

Why do we take ownership of stdin? Is there any benefit to closing the stdin file descriptor?

[ashgti]

I did that while I was trying to figure out how to have cancel request support. I was trying to close the FD to stop the reader thread, but that actually doesn't interrupt in-progress read calls. I should have reverted that when I did some of the Transport refactors. In my cancel request I use the SelectHelper to wait for input now. I can send a PR to change that to false.

[JDevlieghere]

Closing this as this is not a/the right fix.