[lldb-dap] Adding exception handling for dap server disconnect and terminations in lldbdap_testcase.py #155335
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: Piyush Jaiswal (piyushjaiswal98) Changes
Full diff: https://github.com/llvm/llvm-project/pull/155335.diff 1 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index c23b2e73fb45e..8691a87b6fa87 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -466,8 +466,15 @@ def attach(
# if we throw an exception during the test case.
def cleanup():
if disconnectAutomatically:
- self.dap_server.request_disconnect(terminateDebuggee=True)
- self.dap_server.terminate()
+ try:
+ self.dap_server.request_disconnect(terminateDebuggee=True)
+ except (ValueError, TimeoutError, BrokenPipeError, ConnectionError, Exception) as e:
+ # DAP server might not be responsive, skip disconnect and terminate directly
+ print(f"Warning: disconnect failed ({e}), skipping and terminating directly")
+ try:
+ self.dap_server.terminate()
+ except Exception as e:
+ print(f"Warning: terminate failed ({e}), DAP server may have already died")
# Execute the cleanup function during test case tear down.
self.addTearDownHook(cleanup)
@@ -477,9 +484,20 @@ def cleanup():
if expectFailure:
return response
if not (response and response["success"]):
- self.assertTrue(
- response["success"], "attach failed (%s)" % (response["message"])
- )
+ error_msg = "attach failed"
+ if response:
+ if "message" in response:
+ error_msg += " (%s)" % response["message"]
+ elif "body" in response and "error" in response["body"]:
+ if "format" in response["body"]["error"]:
+ error_msg += " (%s)" % response["body"]["error"]["format"]
+ else:
+ error_msg += " (error in body)"
+ else:
+ error_msg += " (no error details available)"
+ else:
+ error_msg += " (no response)"
+ self.assertTrue(response and response["success"], error_msg)
def launch(
self,
|
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
Outdated
Show resolved
Hide resolved
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
Outdated
Show resolved
Hide resolved
Sure...I'll split it into two |
| @@ -466,8 +466,15 @@ def attach( | |||
| # if we throw an exception during the test case. | |||
| def cleanup(): | |||
| if disconnectAutomatically: | |||
| self.dap_server.request_disconnect(terminateDebuggee=True) | |||
| self.dap_server.terminate() | |||
| try: | |||
There was a problem hiding this comment.
We could add both of these as two distinct cleanups. Exceptions thrown in a addTearDownHook are logged already, but I think the goal of this change is to ensure the .terminate is called. The hooks are called in FIFO order, so we could change this to:
if disconnectAutomatically:
self.addTearDownHook(lambda: self.dap_server.request_disconnect(terminateDebuggee=True))
self.addTearDownHook(lambda: self.dap_server.terminate())
There was a problem hiding this comment.
Thanks for the suggestion. I think adding both as separate tear down hooks makes sense. I'll update the PR
There was a problem hiding this comment.
On investigation...addTearDownHook appears to follow a LIFO order...This approach doesn't seem to work... falling back to the original
|
✅ With the latest revision this PR passed the Python code formatter. |
| @@ -450,6 +450,20 @@ def disassemble(self, threadId=None, frameIndex=None): | |||
|
|
|||
| return disassembled_instructions, disassembled_instructions[memoryReference] | |||
|
|
|||
| def dapCleanup(self, disconnectAutomatically): | |||
| if disconnectAutomatically: | |||
| try: | |||
There was a problem hiding this comment.
This is swallowing any exceptions that can occur in either request_disconnect or terminate, which is different behavior than the version originally approved by @walter-erquinigo and @ashgti.
@piyushjaiswal98 says that the teardown hooks are actually processed in LIFO order instead of FIFO order as expected which is why he changed it to catching them. Is there a better way to handle the exceptions here?
Can we use try-finally instead?
def dapCleanup(self, disconnectAutomatically):
try:
if disconnectAutomatically:
self.dap_server.request_disconnect(terminateDebuggee=True)
finally:
self.dap_server.terminate()
Added exception handling to DAP Sever disconnect and termination (Handles multiple types of connection failures and ensures DAP server is always cleaned up, even if disconnect fails)