[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,
|
| self.dap_server.terminate() | ||
| try: | ||
| self.dap_server.request_disconnect(terminateDebuggee=True) | ||
| except (ValueError, TimeoutError, BrokenPipeError, ConnectionError, Exception) as e: |
There was a problem hiding this comment.
If you're going to catch Exception anyway, is there a reason to list the other exceptions?
There was a problem hiding this comment.
This was done documentation intent internally....but it's not needed here...I'll consolidate this
| try: | ||
| self.dap_server.terminate() | ||
| except Exception as e: | ||
| print(f"Warning: terminate failed ({e}), DAP server may have already died") |
There was a problem hiding this comment.
Do we not want the test to fail when this happens?
There was a problem hiding this comment.
This was to make sure the cleanup doesn't fail irrespective of the reason for DAP server crashing (due to the test in question or another cause). Cleanup failing could mask the real the test failure when investigating particular tests
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. |
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)