[lldb-dap] Destroy debugger when debug session terminates #156231
Conversation
|
@llvm/pr-subscribers-lldb Author: Roy Shi (royitaqi) ChangesCurrently, in Server Mode (i.e. Instead, these can be done earlier, during the disconnect request handling. This way, as soon as the debug session is over, logging and release of release of resources can happen. Congestion is also naturally avoided, because it's unlikely that all debug sessions finish at the same time. Full diff: https://github.com/llvm/llvm-project/pull/156231.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp
index 8314a1011a57e..0dfa0b28a20a6 100644
--- a/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DisconnectRequestHandler.cpp
@@ -20,6 +20,11 @@ namespace lldb_dap {
/// Disconnect request; value of command field is 'disconnect'.
Error DisconnectRequestHandler::Run(
const std::optional<DisconnectArguments> &arguments) const {
+ // Destroy the debugger at disconnection. This will trigger the debugger's
+ // destroy callbacks for earlier logging and clean-ups, rather than waiting
+ // until the lldb-dap process's termination.
+ lldb::SBDebugger::Destroy(dap.debugger);
+
bool terminateDebuggee = !dap.is_attach;
if (arguments && arguments->terminateDebuggee)
|
60a32e3 to
c35a97f
Compare
|
I think this is fine. BTW, do we reset the global settings with lldb-dap server mode? |
|
do you know if destroying the debugger will free the module cache? |
@walter-erquinigo: I assume, by "module cache", you meant the global module list accessible by I believe destroying the debugger will NOT free it (which I think is the desired behavior). Verified by the commands below, where the |
|
Good, this looks good to me! |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
FYI @kusmour, @walter-erquinigo, @JDevlieghere, that I have made changes to the patch after you guys approved (btw, thanks for the review). Changes:
I will leave this patch open for another 1-2 days before merging, in case you guys have objections about the new code. --- Details: --- Two things happened after you guys approved the original patch (c35a97f):
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/27286 Here is the relevant piece of the build log for the reference |
|
Trying to understand what test that is and what went wrong. EDIT: Rebuilt and it passed. https://lab.llvm.org/buildbot/#/builders/181/builds/27293 |
Patch
Currently, in Server Mode (i.e.
--connection), all debuggers are destroyed when the lldb-dap process terminates. This causes logging and release of resources to be delayed. This can also cause congestion if multiple debuggers have the same destroy callbacks, which will fight for the same resources (e.g. web requests) at the same time.Instead, the debuggers can be destroyed as early as when the debug session terminates. This way, logging and release of release of resources can happen as soon as possible. Congestion can also be naturally reduced, because it's unlikely that all debug sessions will terminate at the same time.
Tests