[Lldb-commits] [lldb] 76975c7 - Revert "[lldb/Core] Fix a race in the Communication class"

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-04-09T12:49:56+02:00
New Revision: 76975c744da1e82ca6bdbe6883c799340acaf4f0

URL: https://github.com/llvm/llvm-project/commit/76975c744da1e82ca6bdbe6883c799340acaf4f0
DIFF: https://github.com/llvm/llvm-project/commit/76975c744da1e82ca6bdbe6883c799340acaf4f0.diff

LOG: Revert "[lldb/Core] Fix a race in the Communication class"

This reverts commit ebb071345cdae2509c55f9eec76090926fee86a2 -- it seems
to introduce a deadlock in some circumstances.

Added: 
    

Modified: 
    lldb/source/Core/Communication.cpp
    lldb/unittests/Core/CMakeLists.txt

Removed: 
    lldb/unittests/Core/CommunicationTest.cpp


################################################################################
diff  --git a/lldb/source/Core/Communication.cpp b/lldb/source/Core/Communication.cpp
index c3e421191b01..f4163847e4bb 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -315,12 +315,16 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) {
   Status error;
   ConnectionStatus status = eConnectionStatusSuccess;
   bool done = false;
-  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
     size_t bytes_read = comm->ReadFromConnection(
         buf, sizeof(buf), std::chrono::seconds(5), status, &error);
-    if (bytes_read > 0 || status == eConnectionStatusEndOfFile)
+    if (bytes_read > 0)
       comm->AppendBytesToCache(buf, bytes_read, true, status);
+    else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
+      if (comm->GetCloseOnEOF())
+        comm->Disconnect();
+      comm->AppendBytesToCache(buf, bytes_read, true, status);
+    }
 
     switch (status) {
     case eConnectionStatusSuccess:
@@ -328,12 +332,11 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) {
 
     case eConnectionStatusEndOfFile:
       done = true;
-      disconnect = comm->GetCloseOnEOF();
       break;
     case eConnectionStatusError: // Check GetError() for details
       if (error.GetType() == eErrorTypePOSIX && error.GetError() == EIO) {
         // EIO on a pipe is usually caused by remote shutdown
-        disconnect = comm->GetCloseOnEOF();
+        comm->Disconnect();
         done = true;
       }
       if (error.Fail())
@@ -362,15 +365,9 @@ lldb::thread_result_t Communication::ReadThread(lldb::thread_arg_t p) {
   if (log)
     LLDB_LOGF(log, "%p Communication::ReadThread () thread exiting...", p);
 
-  comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
-  {
-    std::lock_guard<std::mutex> guard(comm->m_synchronize_mutex);
-    comm->m_read_thread_did_exit = true;
-    if (disconnect)
-      comm->Disconnect();
-  }
-
+  comm->m_read_thread_did_exit = true;
   // Let clients know that this thread is exiting
+  comm->BroadcastEvent(eBroadcastBitNoMorePendingInput);
   comm->BroadcastEvent(eBroadcastBitReadThreadDidExit);
   return {};
 }

diff  --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt
index 6393c6fe38c2..688a39d9a10f 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -1,5 +1,4 @@
 add_lldb_unittest(LLDBCoreTests
-  CommunicationTest.cpp
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp

diff  --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
deleted file mode 100644
index 6ad0bc720d93..000000000000
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ /dev/null
@@ -1,35 +0,0 @@
-//===-- CommunicationTest.cpp ---------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Core/Communication.h"
-#include "lldb/Host/ConnectionFileDescriptor.h"
-#include "lldb/Host/Pipe.h"
-#include "llvm/Testing/Support/Error.h"
-#include "gtest/gtest.h"
-
-using namespace lldb_private;
-
-TEST(CommunicationTest, SynchronizeWhileClosing) {
-  // Set up a communication object reading from a pipe.
-  Pipe pipe;
-  ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
-                    llvm::Succeeded());
-
-  Communication comm("test");
-  comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
-      pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
-  comm.SetCloseOnEOF(true);
-  ASSERT_TRUE(comm.StartReadThread());
-
-  // Ensure that we can safely synchronize with the read thread while it is
-  // closing the read end (in response to us closing the write end).
-  pipe.CloseWriteFileDescriptor();
-  comm.SynchronizeWithReadThread();
-
-  ASSERT_TRUE(comm.StopReadThread());
-}