mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski, jingham.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.
Ensure that the ConnectionStatus and Status from
Communication::ReadThread() is correctly passed to ::Read() in order
to fix further discrepancies between ::Read() calls in non-threaded
and threaded modes.
Sponsored by: The FreeBSD Foundation
https://reviews.llvm.org/D132577
Files:
lldb/include/lldb/Core/Communication.h
lldb/source/Core/Communication.cpp
lldb/unittests/Core/CommunicationTest.cpp
Index: lldb/unittests/Core/CommunicationTest.cpp
===================================================================
--- lldb/unittests/Core/CommunicationTest.cpp
+++ lldb/unittests/Core/CommunicationTest.cpp
@@ -13,6 +13,9 @@
#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"
+#include <chrono>
+#include <condition_variable>
+#include <mutex>
#include <thread>
#if LLDB_ENABLE_POSIX
@@ -75,9 +78,36 @@
EXPECT_EQ(status, lldb::eConnectionStatusEndOfFile);
EXPECT_THAT_ERROR(error.ToError(), llvm::Succeeded());
- // JoinReadThread() should just return immediately since there was no read
+ // JoinReadThread() should just return immediately if there was no read
// thread started.
EXPECT_TRUE(comm.JoinReadThread());
+
+ // Test using Communication without a connection.
+ comm.SetConnection(nullptr);
+ if (use_read_thread)
+ ASSERT_TRUE(comm.StartReadThread());
+ error.Clear();
+ EXPECT_EQ(
+ comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 0U);
+ EXPECT_EQ(status, lldb::eConnectionStatusNoConnection);
+ EXPECT_THAT_ERROR(error.ToError(), llvm::Failed());
+ EXPECT_TRUE(comm.JoinReadThread());
+
+ // Test using Communication that is disconnected.
+ ASSERT_THAT_ERROR(pipe.CreateNew(/*child_process_inherit=*/false).ToError(),
+ llvm::Succeeded());
+ comm.SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ pipe.ReleaseReadFileDescriptor(), /*owns_fd=*/true));
+ comm.SetCloseOnEOF(true);
+ ASSERT_EQ(comm.Disconnect(), lldb::eConnectionStatusSuccess);
+ if (use_read_thread)
+ ASSERT_TRUE(comm.StartReadThread());
+ error.Clear();
+ EXPECT_EQ(
+ comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 0U);
+ EXPECT_EQ(status, lldb::eConnectionStatusLostConnection);
+ EXPECT_THAT_ERROR(error.ToError(), llvm::Failed());
+ EXPECT_TRUE(comm.JoinReadThread());
}
TEST(CommunicationTest, Read) {
@@ -88,6 +118,31 @@
CommunicationReadTest(/*use_thread=*/true);
}
+TEST(CommunicationTest, StopReadThread) {
+ std::condition_variable finished;
+ std::mutex finished_mutex;
+
+ std::thread test_thread{[&finished]() {
+ 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());
+ ASSERT_TRUE(comm.StopReadThread());
+ finished.notify_all();
+ }};
+
+ // StopReadThread() can hang, so force an external timeout.
+ std::unique_lock<std::mutex> lock{finished_mutex};
+ ASSERT_EQ(finished.wait_for(lock, std::chrono::seconds(3)), std::cv_status::no_timeout);
+ test_thread.join();
+}
+
TEST(CommunicationTest, SynchronizeWhileClosing) {
// Set up a communication object reading from a pipe.
Pipe pipe;
Index: lldb/source/Core/Communication.cpp
===================================================================
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -162,14 +162,10 @@
if (event_type & eBroadcastBitReadThreadDidExit) {
// If the thread exited of its own accord, it either means it
- // hit an end-of-file condition or lost connection
- // (we verified that we had an connection above).
- if (!m_connection_sp) {
- if (error_ptr)
- error_ptr->SetErrorString("Lost connection.");
- status = eConnectionStatusLostConnection;
- } else
- status = eConnectionStatusEndOfFile;
+ // hit an end-of-file condition or an error.
+ status = m_pass_status;
+ if (error_ptr)
+ *error_ptr = std::move(m_pass_error);
if (GetCloseOnEOF())
Disconnect(nullptr);
@@ -384,7 +380,8 @@
break;
}
}
- log = GetLog(LLDBLog::Communication);
+ m_pass_status = status;
+ m_pass_error = std::move(error);
LLDB_LOG(log, "Communication({0}) thread exiting...", this);
// Handle threads wishing to synchronize with us.
Index: lldb/include/lldb/Core/Communication.h
===================================================================
--- lldb/include/lldb/Core/Communication.h
+++ lldb/include/lldb/Core/Communication.h
@@ -324,6 +324,9 @@
m_bytes; ///< A buffer to cache bytes read in the ReadThread function.
std::recursive_mutex m_bytes_mutex; ///< A mutex to protect multi-threaded
///access to the cached bytes.
+ lldb::ConnectionStatus m_pass_status; ///< Connection status passthrough
+ ///from read thread.
+ Status m_pass_error; ///< Error passthrough from read thread.
std::mutex
m_write_mutex; ///< Don't let multiple threads write at the same time...
std::mutex m_synchronize_mutex;
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
