[Lldb-commits] [lldb] [lldb-dap] Fix a race during shutdown (PR #91591)

[Pavel Labath via lldb-commits]

https://github.com/labath created https://github.com/llvm/llvm-project/pull/91591

lldb-dap was setting a flag which was meant to shut it down as soon as it sent a terminated event. The problem with this flag is two-fold:
- as far as I can tell (definitely not an expert here), there's no justification for this in the protocol spec. The only way I found to shut the server down was to send it a disconnect request.
- the flag did not actually work most of the time, because it's only checked between requests so nothing will happen if the server starts listening for a new request before a different thread manages to send the terminated event. And since the next request is usually the disconnect request, everything will operate normally.

The combination of these two things meant that the issue was largely unnoticable, except for rare flaky test failures, which happened when the handler thread was too slow, and checked the flag after it has already been said. This caused the test suite to complain as it did not get a response to the disconnect request. This situation could be s(t)imulated by adding a sleep to the and of the main loop, which delayed the flag check, and caused the DAP tests to fail reliably.

This patch changes the shutdown condition to only trigger when the disconnect request has been received. Since the flag can now only be set from the handler thread, it no longer needs to be atomic.

>From 2d9aaa5260f9a7b00563c84a111d81050518851c Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Thu, 9 May 2024 13:10:53 +0000
Subject: [PATCH] [lldb-dap] Fix a race during shutdown

lldb-dap was setting a flag which was meant to shut it down as soon as
it sent a terminated event. The problem with this flag is two-fold:
- as far as I can tell (definitely not an expert here), there's no
  justification for this in the protocol spec. The only way I found to
  shut the server down was to send it a disconnect request.
- the flag did not actually work most of the time, because it's only
  checked between requests so nothing will happen if the server starts
  listening for a new request before a different thread manages to send
  the terminated event. And since the next request is usually the
  disconnect request, everything will operate normally.

The combination of these two things meant that the issue was largely
unnoticable, except for rare flaky test failures, which happened when
the handler thread was too slow, and checked the flag after it has
already been said. This caused the test suite to complain as it did not
get a response to the disconnect request. This situation could be
s(t)imulated by adding a sleep to the and of the main loop, which
delayed the flag check, and caused the DAP tests to fail reliably.

This patch changes the shutdown condition to only trigger when the
disconnect request has been received. Since the flag can now only be set
from the handler thread, it no longer needs to be atomic.
---
 lldb/tools/lldb-dap/DAP.cpp      |  5 ++---
 lldb/tools/lldb-dap/DAP.h        |  2 +-
 lldb/tools/lldb-dap/lldb-dap.cpp | 25 +++++++------------------
 3 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index b254ddfef0d5f..55ff1493c1011 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -39,8 +39,7 @@ DAP::DAP()
            {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
            {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
            {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
-      focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
-      stop_at_entry(false), is_attach(false),
+      focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
       enable_auto_variable_summaries(false),
       enable_synthetic_child_debugging(false),
       restarting_process_id(LLDB_INVALID_PROCESS_ID),
@@ -623,7 +622,7 @@ bool DAP::HandleObject(const llvm::json::Object &object) {
 }
 
 llvm::Error DAP::Loop() {
-  while (!sent_terminated_event) {
+  while (!disconnecting) {
     llvm::json::Object object;
     lldb_dap::PacketStatus status = GetNextObject(object);
 
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 5c70a056fea4b..bbd9d46ba3a04 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -168,7 +168,7 @@ struct DAP {
   // arguments if we get a RestartRequest.
   std::optional<llvm::json::Object> last_launch_or_attach_request;
   lldb::tid_t focus_tid;
-  std::atomic<bool> sent_terminated_event;
+  bool disconnecting = false;
   bool stop_at_entry;
   bool is_attach;
   bool enable_auto_variable_summaries;
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index f35abd665e844..96da458be21d1 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -226,26 +226,14 @@ void SendContinuedEvent() {
 // Send a "terminated" event to indicate the process is done being
 // debugged.
 void SendTerminatedEvent() {
-  // If an inferior exits prior to the processing of a disconnect request, then
-  // the threads executing EventThreadFunction and request_discontinue
-  // respectively may call SendTerminatedEvent simultaneously. Without any
-  // synchronization, the thread executing EventThreadFunction may set
-  // g_dap.sent_terminated_event before the thread executing
-  // request_discontinue has had a chance to test it, in which case the latter
-  // would move ahead to issue a response to the disconnect request. Said
-  // response may get dispatched ahead of the terminated event compelling the
-  // client to terminate the debug session without consuming any console output
-  // that might've been generated by the execution of terminateCommands. So,
-  // synchronize simultaneous calls to SendTerminatedEvent.
+  // Prevent races if the process exits while we're being asked to disconnect.
   static std::mutex mutex;
   std::lock_guard<std::mutex> locker(mutex);
-  if (!g_dap.sent_terminated_event) {
-    g_dap.sent_terminated_event = true;
-    g_dap.RunTerminateCommands();
-    // Send a "terminated" event
-    llvm::json::Object event(CreateTerminatedEventObject());
-    g_dap.SendJSON(llvm::json::Value(std::move(event)));
-  }
+
+  g_dap.RunTerminateCommands();
+  // Send a "terminated" event
+  llvm::json::Object event(CreateTerminatedEventObject());
+  g_dap.SendJSON(llvm::json::Value(std::move(event)));
 }
 
 // Send a thread stopped event for all threads as long as the process
@@ -1003,6 +991,7 @@ void request_disconnect(const llvm::json::Object &request) {
     g_dap.broadcaster.BroadcastEventByType(eBroadcastBitStopProgressThread);
     g_dap.progress_event_thread.join();
   }
+  g_dap.disconnecting = true;
 }
 
 void request_exceptionInfo(const llvm::json::Object &request) {