[lldb-dap] Ensure we acquire the SB API lock while handling requests. #137026
Conversation
Aquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesAcquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread. Full diff: https://github.com/llvm/llvm-project/pull/137026.diff 1 Files Affected:
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 3520dc2c71a55..be9273963654a 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -14,6 +14,7 @@
#include "Protocol/ProtocolBase.h"
#include "RunInTerminal.h"
#include "llvm/Support/Error.h"
+#include <mutex>
#if !defined(_WIN32)
#include <unistd.h>
@@ -180,6 +181,9 @@ void BaseRequestHandler::Run(const Request &request) {
return;
}
+ lldb::SBMutex lock = dap.GetAPIMutex();
+ std::lock_guard<lldb::SBMutex> guard(lock);
+
// FIXME: After all the requests have migrated from LegacyRequestHandler >
// RequestHandler<> we should be able to move this into
// RequestHandler<>::operator().
|
…llvm#137026) Acquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread.
…llvm#137026) Acquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread.
…llvm#137026) Acquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread.
|
@ashgti, this change is causing deadlock in our lldb. We have internal changes that main thread's disconnect request handler tried to reset debugger states and call StopEventHandlers() to ensure all event threads to stop. However, this deadlocks now because event thread can call Main Thread Event thread: High level, I do not think it is a good idea to hold the top level API mutex which is way too large locking scope. We should leave the decision to each request handler for smaller scope locks if they want to ensure a critical section. Can we revert the PR? cc @clayborg |
|
We could revert this and try to add more specific locks in request handlers instead. |
FWIW I don't necessarily agree with this observation. While I agree that a smaller critical section is better, conceptually a single request corresponds to a group of SB API calls. Making it the responsibility of the requests to group all SB API calls within the scope of the lock is a lot more error prone. What is the other lock involved in the deadlock, other than the API mutex? |
It is the same lock ( |
|
We could move the join to when the DAP::Loop exits, that would unblock the event handler, I think. |
|
So, I was checking this and I see That function doesn't exist in the open source version of lldb-dap. I'm not sure what its doing but it looks like its trying to join the event thread. The llvm-project/lldb/tools/lldb-dap/DAP.cpp Lines 935 to 939 in 3275291 I'm not sure I have enough information on this deadlock. Is the |
|
As I mentioned in the first comment, In my opinion, blindly hold a lock during the lifetime of request handling is unnecessary restrictive. It will prevent any synchronization between request thread to another thread if the other thread can potentially call public API. To justify it for less error prone is like saying holding a global lock so that multithreading programming can be less racing. We have to be more flexible and think of the trade-off here. |
Acquiring the lock for the target should help ensure consistency with other background operations, like the thread monitoring events that can trigger run commands from a different thread.