[lldb] Refactor JSONTransport own MainLoop read handle. - #179564
Merged
Conversation
When working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the `MessageHandler::OnClose`. Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle. This change moves the ownership of the `MainLoop::ReadHandleUP` and additionally own a reference to the `MainLoop` itself to ensure the loop outlives the JSONTransport object. This allows us to remove the handle immediately when we detect an EOF / hang up has occurred.
Member
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesWhen working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle. This change moves the ownership of the This allows us to remove the handle immediately when we detect an EOF / hang up has occurred. Full diff: https://github.com/llvm/llvm-project/pull/179564.diff 15 Files Affected:
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
index bd41d077c903b..6b114ee497a8b 100644
--- a/lldb/include/lldb/Host/JSONTransport.h
+++ b/lldb/include/lldb/Host/JSONTransport.h
@@ -166,8 +166,7 @@ class JSONTransport {
///
/// If an unexpected error occurs, the MainLoop will be terminated and a log
/// message will include additional information about the termination reason.
- virtual llvm::Expected<MainLoop::ReadHandleUP>
- RegisterMessageHandler(MainLoop &loop, MessageHandler &handler) = 0;
+ virtual llvm::Error RegisterMessageHandler(MessageHandler &handler) = 0;
protected:
template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
@@ -182,29 +181,27 @@ template <typename Proto> class IOTransport : public JSONTransport<Proto> {
using Message = typename JSONTransport<Proto>::Message;
using MessageHandler = typename JSONTransport<Proto>::MessageHandler;
- IOTransport(lldb::IOObjectSP in, lldb::IOObjectSP out)
- : m_in(in), m_out(out) {}
+ IOTransport(MainLoop &loop, lldb::IOObjectSP in, lldb::IOObjectSP out)
+ : m_loop(loop), m_in(in), m_out(out) {}
llvm::Error Send(const typename Proto::Evt &evt) override {
return Write(evt);
}
+
llvm::Error Send(const typename Proto::Req &req) override {
return Write(req);
}
+
llvm::Error Send(const typename Proto::Resp &resp) override {
return Write(resp);
}
- llvm::Expected<MainLoop::ReadHandleUP>
- RegisterMessageHandler(MainLoop &loop, MessageHandler &handler) override {
+ llvm::Error RegisterMessageHandler(MessageHandler &handler) override {
Status status;
- MainLoop::ReadHandleUP read_handle = loop.RegisterReadObject(
+ m_read_handle = m_loop.RegisterReadObject(
m_in, [this, &handler](MainLoopBase &base) { OnRead(base, handler); },
status);
- if (status.Fail()) {
- return status.takeError();
- }
- return read_handle;
+ return status.takeError();
}
/// Public for testing purposes, otherwise this should be an implementation
@@ -263,11 +260,15 @@ template <typename Proto> class IOTransport : public JSONTransport<Proto> {
handler.OnError(llvm::make_error<TransportUnhandledContentsError>(
std::string(m_buffer.str())));
handler.OnClosed();
+ // On EOF, remove the read handle from the MainLoop.
+ m_read_handle.reset();
}
}
+ MainLoop &m_loop;
lldb::IOObjectSP m_in;
lldb::IOObjectSP m_out;
+ MainLoop::ReadHandleUP m_read_handle;
};
/// A transport class for JSON with a HTTP header.
diff --git a/lldb/include/lldb/Protocol/MCP/Server.h b/lldb/include/lldb/Protocol/MCP/Server.h
index f185d51f41192..4801352e8154b 100644
--- a/lldb/include/lldb/Protocol/MCP/Server.h
+++ b/lldb/include/lldb/Protocol/MCP/Server.h
@@ -70,7 +70,6 @@ class Server {
LogCallback m_log_callback;
struct Client {
- ReadHandleUP handle;
MCPTransportUP transport;
MCPBinderUP binder;
};
diff --git a/lldb/include/lldb/Protocol/MCP/Transport.h b/lldb/include/lldb/Protocol/MCP/Transport.h
index b7a1eb778d660..ceadf1dbd82b8 100644
--- a/lldb/include/lldb/Protocol/MCP/Transport.h
+++ b/lldb/include/lldb/Protocol/MCP/Transport.h
@@ -83,8 +83,8 @@ using LogCallback = llvm::unique_function<void(llvm::StringRef message)>;
class Transport final
: public lldb_private::transport::JSONRPCTransport<ProtocolDescriptor> {
public:
- Transport(lldb::IOObjectSP in, lldb::IOObjectSP out,
- LogCallback log_callback = {});
+ Transport(lldb_private::MainLoop &loop, lldb::IOObjectSP in,
+ lldb::IOObjectSP out, LogCallback log_callback = {});
virtual ~Transport() = default;
/// Transport is not copyable.
diff --git a/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp b/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
index 77a3ba6574cde..23c23adcd96b1 100644
--- a/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
+++ b/lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
@@ -66,7 +66,7 @@ void ProtocolServerMCP::AcceptCallback(std::unique_ptr<Socket> socket) {
lldb::IOObjectSP io_sp = std::move(socket);
auto transport_up = std::make_unique<lldb_protocol::mcp::Transport>(
- io_sp, io_sp, [client_name](llvm::StringRef message) {
+ m_loop, io_sp, io_sp, [client_name](llvm::StringRef message) {
LLDB_LOG(GetLog(LLDBLog::Host), "{0}: {1}", client_name, message);
});
diff --git a/lldb/source/Protocol/MCP/Server.cpp b/lldb/source/Protocol/MCP/Server.cpp
index a8871ff5c39f0..e305e78fa1084 100644
--- a/lldb/source/Protocol/MCP/Server.cpp
+++ b/lldb/source/Protocol/MCP/Server.cpp
@@ -156,12 +156,10 @@ llvm::Error Server::Accept(MainLoop &loop, MCPTransportUP transport) {
Logv("Transport error: {0}", llvm::toString(std::move(err)));
});
- auto handle = transport->RegisterMessageHandler(loop, *binder);
- if (!handle)
- return handle.takeError();
+ if (llvm::Error err = transport->RegisterMessageHandler(*binder))
+ return err;
- m_instances[transport_ptr] =
- Client{std::move(*handle), std::move(transport), std::move(binder)};
+ m_instances[transport_ptr] = Client{std::move(transport), std::move(binder)};
return llvm::Error::success();
}
diff --git a/lldb/source/Protocol/MCP/Transport.cpp b/lldb/source/Protocol/MCP/Transport.cpp
index cccdc3b5bd65c..1dc01a9f59008 100644
--- a/lldb/source/Protocol/MCP/Transport.cpp
+++ b/lldb/source/Protocol/MCP/Transport.cpp
@@ -13,9 +13,10 @@
using namespace lldb_protocol::mcp;
using namespace llvm;
-Transport::Transport(lldb::IOObjectSP in, lldb::IOObjectSP out,
- LogCallback log_callback)
- : JSONRPCTransport(in, out), m_log_callback(std::move(log_callback)) {}
+Transport::Transport(lldb_private::MainLoop &loop, lldb::IOObjectSP in,
+ lldb::IOObjectSP out, LogCallback log_callback)
+ : JSONRPCTransport(loop, in, out), m_log_callback(std::move(log_callback)) {
+}
void Transport::Log(StringRef message) {
if (m_log_callback)
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 5ef384706aab0..42b4c3fd86d84 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -1046,9 +1046,8 @@ void DAP::TransportHandler() {
m_queue_cv.notify_all();
});
- auto handle = transport.RegisterMessageHandler(m_loop, *this);
- if (!handle) {
- DAP_LOG_ERROR(log, handle.takeError(),
+ if (llvm::Error err = transport.RegisterMessageHandler(*this)) {
+ DAP_LOG_ERROR(log, std::move(err),
"registering message handler failed: {0}");
std::lock_guard<std::mutex> guard(m_queue_mutex);
m_error_occurred = true;
diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp
index b3512385d6579..b149a8ee8f026 100644
--- a/lldb/tools/lldb-dap/Transport.cpp
+++ b/lldb/tools/lldb-dap/Transport.cpp
@@ -17,9 +17,9 @@ using namespace lldb_private;
namespace lldb_dap {
-Transport::Transport(lldb_dap::Log &log, lldb::IOObjectSP input,
- lldb::IOObjectSP output)
- : HTTPDelimitedJSONTransport(input, output), m_log(log) {}
+Transport::Transport(lldb_dap::Log &log, lldb_private::MainLoop &loop,
+ lldb::IOObjectSP input, lldb::IOObjectSP output)
+ : HTTPDelimitedJSONTransport(loop, input, output), m_log(log) {}
void Transport::Log(llvm::StringRef message) {
// Emit the message directly, since this log was forwarded.
diff --git a/lldb/tools/lldb-dap/Transport.h b/lldb/tools/lldb-dap/Transport.h
index b20a93475d2dd..42f7caf93831a 100644
--- a/lldb/tools/lldb-dap/Transport.h
+++ b/lldb/tools/lldb-dap/Transport.h
@@ -35,8 +35,8 @@ class Transport final
: public lldb_private::transport::HTTPDelimitedJSONTransport<
ProtocolDescriptor> {
public:
- Transport(lldb_dap::Log &log, lldb::IOObjectSP input,
- lldb::IOObjectSP output);
+ Transport(lldb_dap::Log &log, lldb_private::MainLoop &loop,
+ lldb::IOObjectSP input, lldb::IOObjectSP output);
virtual ~Transport() = default;
void Log(llvm::StringRef message) override;
diff --git a/lldb/tools/lldb-dap/tool/lldb-dap.cpp b/lldb/tools/lldb-dap/tool/lldb-dap.cpp
index 15c63543e86f5..babc3c98646cb 100644
--- a/lldb/tools/lldb-dap/tool/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/tool/lldb-dap.cpp
@@ -47,7 +47,6 @@
#include "llvm/Support/Threading.h"
#include "llvm/Support/WithColor.h"
#include "llvm/Support/raw_ostream.h"
-#include <condition_variable>
#include <cstddef>
#include <cstdio>
#include <cstdlib>
@@ -463,7 +462,7 @@ static llvm::Error serveConnection(
DAP_LOG(client_log, "client connected");
MainLoop loop;
- Transport transport(client_log, io, io);
+ Transport transport(client_log, loop, io, io);
DAP dap(client_log, default_repl_mode, pre_init_commands, no_lldbinit,
client_name, transport, loop);
@@ -738,7 +737,7 @@ int main(int argc, char *argv[]) {
constexpr llvm::StringLiteral client_name = "stdio";
MainLoop loop;
Log client_log = log.WithPrefix("(stdio)");
- Transport transport(client_log, input, output);
+ Transport transport(client_log, loop, input, output);
DAP dap(client_log, default_repl_mode, pre_init_commands, no_lldbinit,
client_name, transport, loop);
diff --git a/lldb/unittests/DAP/TestBase.cpp b/lldb/unittests/DAP/TestBase.cpp
index a9231085637c9..1afac18833a03 100644
--- a/lldb/unittests/DAP/TestBase.cpp
+++ b/lldb/unittests/DAP/TestBase.cpp
@@ -35,7 +35,7 @@ using lldb_private::MainLoop;
using lldb_private::Pipe;
void TransportBase::SetUp() {
- std::tie(to_client, to_server) = TestDAPTransport::createPair();
+ std::tie(to_client, to_server) = TestDAPTransport::createPair(loop);
log = std::make_unique<Log>(llvm::outs(), log_mutex);
dap = std::make_unique<DAP>(
@@ -46,13 +46,8 @@ void TransportBase::SetUp() {
/*client_name=*/"test_client",
/*transport=*/*to_client, /*loop=*/loop);
- auto server_handle = to_server->RegisterMessageHandler(loop, *dap);
- EXPECT_THAT_EXPECTED(server_handle, Succeeded());
- handles[0] = std::move(*server_handle);
-
- auto client_handle = to_client->RegisterMessageHandler(loop, client);
- EXPECT_THAT_EXPECTED(client_handle, Succeeded());
- handles[1] = std::move(*client_handle);
+ EXPECT_THAT_ERROR(to_server->RegisterMessageHandler(*dap), Succeeded());
+ EXPECT_THAT_ERROR(to_client->RegisterMessageHandler(client), Succeeded());
}
void TransportBase::Run() {
diff --git a/lldb/unittests/DAP/TestBase.h b/lldb/unittests/DAP/TestBase.h
index f1c7e6b989729..c354829377434 100644
--- a/lldb/unittests/DAP/TestBase.h
+++ b/lldb/unittests/DAP/TestBase.h
@@ -59,7 +59,6 @@ class TransportBase : public testing::Test {
lldb_private::SubsystemRAII<lldb_private::FileSystem, lldb_private::HostInfo>
subsystems;
lldb_private::MainLoop loop;
- lldb_private::MainLoop::ReadHandleUP handles[2];
std::unique_ptr<lldb_dap::Log> log;
lldb_dap::Log::Mutex log_mutex;
diff --git a/lldb/unittests/Host/JSONTransportTest.cpp b/lldb/unittests/Host/JSONTransportTest.cpp
index 710907af3794a..2c26f94213773 100644
--- a/lldb/unittests/Host/JSONTransportTest.cpp
+++ b/lldb/unittests/Host/JSONTransportTest.cpp
@@ -247,19 +247,22 @@ template <typename T> class JSONTransportTest : public PipePairTest {
protected:
SubsystemRAII<FileSystem> subsystems;
+ MainLoop loop;
test_protocol::MessageHandler message_handler;
std::unique_ptr<T> transport;
- MainLoop loop;
void SetUp() override {
PipePairTest::SetUp();
transport = std::make_unique<T>(
- std::make_shared<NativeFile>(input.GetReadFileDescriptor(),
+ loop,
+ std::make_shared<NativeFile>(input.ReleaseReadFileDescriptor(),
File::eOpenOptionReadOnly,
- NativeFile::Unowned),
- std::make_shared<NativeFile>(output.GetWriteFileDescriptor(),
+ NativeFile::Owned),
+ std::make_shared<NativeFile>(output.ReleaseWriteFileDescriptor(),
File::eOpenOptionWriteOnly,
- NativeFile::Unowned));
+ NativeFile::Owned));
+ EXPECT_THAT_ERROR(transport->RegisterMessageHandler(message_handler),
+ Succeeded());
}
/// Run the transport MainLoop and return any messages received.
@@ -272,17 +275,13 @@ template <typename T> class JSONTransportTest : public PipePairTest {
loop.RequestTermination();
});
}
- bool addition_succeeded = loop.AddCallback(
+ bool registered_timeout = loop.AddCallback(
[](MainLoopBase &loop) {
loop.RequestTermination();
FAIL() << "timeout";
},
timeout);
- EXPECT_TRUE(addition_succeeded);
- auto handle = transport->RegisterMessageHandler(loop, message_handler);
- if (!handle)
- return handle.takeError();
-
+ EXPECT_TRUE(registered_timeout);
return loop.Run().takeError();
}
@@ -360,14 +359,13 @@ class TransportBinderTest : public testing::Test {
MainLoop loop;
void SetUp() override {
- std::tie(to_remote, from_remote) = test_protocol::Transport::createPair();
+ std::tie(to_remote, from_remote) =
+ test_protocol::Transport::createPair(loop);
binder = std::make_unique<test_protocol::Binder>(*to_remote);
- auto binder_handle = to_remote->RegisterMessageHandler(loop, remote);
- EXPECT_THAT_EXPECTED(binder_handle, Succeeded());
-
- auto remote_handle = from_remote->RegisterMessageHandler(loop, *binder);
- EXPECT_THAT_EXPECTED(remote_handle, Succeeded());
+ EXPECT_THAT_ERROR(to_remote->RegisterMessageHandler(remote), Succeeded());
+ EXPECT_THAT_ERROR(from_remote->RegisterMessageHandler(*binder),
+ Succeeded());
}
void Run() {
@@ -502,8 +500,8 @@ TEST_F(HTTPDelimitedJSONTransportTest, ReaderWithUnhandledData) {
TEST_F(HTTPDelimitedJSONTransportTest, InvalidTransport) {
transport =
- std::make_unique<TestHTTPDelimitedJSONTransport>(nullptr, nullptr);
- ASSERT_THAT_ERROR(Run(/*close_input=*/false),
+ std::make_unique<TestHTTPDelimitedJSONTransport>(loop, nullptr, nullptr);
+ ASSERT_THAT_ERROR(transport->RegisterMessageHandler(message_handler),
FailedWithMessage("IO object is not valid."));
}
@@ -624,8 +622,8 @@ TEST_F(JSONRPCTransportTest, Write) {
}
TEST_F(JSONRPCTransportTest, InvalidTransport) {
- transport = std::make_unique<TestJSONRPCTransport>(nullptr, nullptr);
- ASSERT_THAT_ERROR(Run(/*close_input=*/false),
+ transport = std::make_unique<TestJSONRPCTransport>(loop, nullptr, nullptr);
+ ASSERT_THAT_ERROR(transport->RegisterMessageHandler(message_handler),
FailedWithMessage("IO object is not valid."));
}
diff --git a/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp b/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
index 97f32e2fbb1bf..9a5b75edeeb9d 100644
--- a/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
+++ b/lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
@@ -157,22 +157,18 @@ class ProtocolServerMCPTest : public testing::Test {
}
void SetUp() override {
- std::tie(to_client, to_server) = Transport::createPair();
+ std::tie(to_client, to_server) = Transport::createPair(loop);
server_up = std::make_unique<TestServer>(
"lldb-mcp", "0.1.0",
[this](StringRef msg) { logged_messages.push_back(msg.str()); });
binder = server_up->Bind(*to_client);
- auto server_handle = to_server->RegisterMessageHandler(loop, *binder);
- EXPECT_THAT_EXPECTED(server_handle, Succeeded());
binder->OnError([](llvm::Error error) {
llvm::errs() << formatv("Server transport error: {0}", error);
});
- handles[0] = std::move(*server_handle);
- auto client_handle = to_client->RegisterMessageHandler(loop, client);
- EXPECT_THAT_EXPECTED(client_handle, Succeeded());
- handles[1] = std::move(*client_handle);
+ EXPECT_THAT_ERROR(to_server->RegisterMessageHandler(*binder), Succeeded());
+ EXPECT_THAT_ERROR(to_client->RegisterMessageHandler(client), Succeeded());
}
template <typename Result, typename Params>
diff --git a/lldb/unittests/TestingSupport/Host/JSONTransportTestUtilities.h b/lldb/unittests/TestingSupport/Host/JSONTransportTestUtilities.h
index bacf8ca36aa07..4623c365c960f 100644
--- a/lldb/unittests/TestingSupport/Host/JSONTransportTestUtilities.h
+++ b/lldb/unittests/TestingSupport/Host/JSONTransportTestUtilities.h
@@ -30,72 +30,51 @@ class TestTransport final
static std::pair<std::unique_ptr<TestTransport<Proto>>,
std::unique_ptr<TestTransport<Proto>>>
- createPair() {
+ createPair(lldb_private::MainLoop &loop) {
std::unique_ptr<TestTransport<Proto>> transports[2] = {
- std::make_unique<TestTransport<Proto>>(),
- std::make_unique<TestTransport<Proto>>()};
+ std::make_unique<TestTransport<Proto>>(loop),
+ std::make_unique<TestTransport<Proto>>(loop)};
return std::make_pair(std::move(transports[0]), std::move(transports[1]));
}
- explicit TestTransport() {
- llvm::Expected<lldb::FileUP> dummy_file =
- lldb_private::FileSystem::Instance().Open(
- lldb_private::FileSpec(lldb_private::FileSystem::DEV_NULL),
- lldb_private::File::eOpenOptionReadWrite);
- EXPECT_THAT_EXPECTED(dummy_file, llvm::Succeeded());
- m_dummy_file = std::move(*dummy_file);
- }
+ explicit TestTransport(lldb_private::MainLoop &loop) : m_loop(loop) {}
llvm::Error Send(const typename Proto::Evt &evt) override {
- EXPECT_TRUE(m_loop && m_handler)
- << "Send called before RegisterMessageHandler";
- m_loop->AddPendingCallback([this, evt](lldb_private::MainLoopBase &) {
+ EXPECT_TRUE(m_handler) << "Send called before RegisterMessageHandler";
+ m_loop.AddPendingCallback([this, evt](lldb_private::MainLoopBase &) {
m_handler->Received(evt);
});
return llvm::Error::success();
}
llvm::Error Send(const typename Proto::Req &req) override {
- EXPECT_TRUE(m_loop && m_handler)
- << "Send called before RegisterMessageHandler";
- m_loop->AddPendingCallback([this, req](lldb_private::MainLoopBase &) {
+ EXPECT_TRUE(m_handler) << "Send called before RegisterMessageHandler";
+ m_loop.AddPendingCallback([this, req](lldb_private::MainLoopBase &) {
m_handler->Received(req);
});
return llvm::Error::success();
}
llvm::Error Send(const typename Proto::Resp &resp) override {
- EXPECT_TRUE(m_loop && m_handler)
- << "Send called before RegisterMessageHandler";
- m_loop->AddPendingCallback([this, resp](lldb_private::MainLoopBase &) {
+ EXPECT_TRUE(m_handler) << "Send called before RegisterMessageHandler";
+ m_loop.AddPendingCallback([this, resp](lldb_private::MainLoopBase &) {
m_handler->Received(resp);
});
return llvm::Error::success();
}
- llvm::Expected<lldb_private::MainLoop::ReadHandleUP>
- RegisterMessageHandler(lldb_private::MainLoop &loop,
- MessageHandler &handler) override {
- if (!m_loop)
- m_loop = &loop;
+ llvm::Error RegisterMessageHandler(MessageHandler &handler) override {
if (!m_handler)
m_handler = &handler;
- lldb_private::Status status;
- auto handle = loop.RegisterReadObject(
- m_dummy_file, [](lldb_private::MainLoopBase &) {}, status);
- if (status.Fail())
- return status.takeError();
- return handle;
+ return llvm::Error::success();
}
protected:
void Log(llvm::StringRef message) override {};
private:
- lldb_private::MainLoop *m_loop = nullptr;
+ lldb_private::MainLoop &m_loop;
MessageHandler *m_handler = nullptr;
- // Dummy file for registering with the MainLoop.
- lldb::FileSP m_dummy_file = nullptr;
};
template <typename Proto>
|
This was referenced Feb 3, 2026
da-viper
reviewed
Feb 5, 2026
Comment on lines
+184
to
206
| IOTransport(MainLoop &loop, lldb::IOObjectSP in, lldb::IOObjectSP out) | ||
| : m_loop(loop), m_in(in), m_out(out) {} | ||
|
|
||
| llvm::Error Send(const typename Proto::Evt &evt) override { | ||
| return Write(evt); | ||
| } | ||
|
|
||
| llvm::Error Send(const typename Proto::Req &req) override { | ||
| return Write(req); | ||
| } | ||
|
|
||
| llvm::Error Send(const typename Proto::Resp &resp) override { | ||
| return Write(resp); | ||
| } | ||
|
|
||
| llvm::Expected<MainLoop::ReadHandleUP> | ||
| RegisterMessageHandler(MainLoop &loop, MessageHandler &handler) override { | ||
| llvm::Error RegisterMessageHandler(MessageHandler &handler) override { | ||
| Status status; | ||
| MainLoop::ReadHandleUP read_handle = loop.RegisterReadObject( | ||
| m_read_handle = m_loop.RegisterReadObject( | ||
| m_in, [this, &handler](MainLoopBase &base) { OnRead(base, handler); }, | ||
| status); | ||
| if (status.Fail()) { | ||
| return status.takeError(); | ||
| } | ||
| return read_handle; | ||
| return status.takeError(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
I don't see a reason we heed to hold a reference to the Mainloop as we get it from registerReadObject and only use it there. especially since the MCP Server::Accept provides one and we have one in DAP. Unless I am missing something.
Contributor
Author
There was a problem hiding this comment.
The reason I keep it around is to ensure the MainLoop out lives the JSONTransport object. If we took a pointer or if we only call RegisterReadObject from RegisterMessageHandler we can end up with a transport out living the MainLoop which caused some crashes for me in the unit tests.
Contributor
There was a problem hiding this comment.
If we took a pointer or if we only call RegisterReadObject from RegisterMessageHandler
There is no difference constructing the JSONTransport with a MainLoop pointer instead of a reference as it is the same thing, we just end up with a 'dangling reference' instead of a 'dangling pointer'. The JSONTransport class doesn't own or clean up the MainLoop reference member, m_loop will get destroyed once it does out of scope where the it was originally created.
The JSONTransport class should out live the MainLoop's event loop since we are now holding a handle to ReadObject. Unless something calls RequestTermination.
Contributor
Author
There was a problem hiding this comment.
I wanted to let the JSONTransport take the MainLoop in so it can be used cooperatively in the MainLoop with other items instead of fully owning it internally. In theory, when lldb-dap is running in server mode we could use a shared loop for running client requests. Currently, we spawn a thread for each but we could adjust that behavior.
We should be cleaning up the handle when the transport is destructed, that may or may not happen after a termination request. Not every session is terminated cleanly (e.g. the remote could simply hangup when running in server mode without a termination request).
da-viper
reviewed
Feb 5, 2026
…t have been updated.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Feb 6, 2026
When working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the `MessageHandler::OnClose`. Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle. This change moves the ownership of the `MainLoop::ReadHandleUP` and additionally own a reference to the `MainLoop` itself to ensure the loop outlives the JSONTransport object. This allows us to remove the handle immediately when we detect an EOF / hang up has occurred.
rishabhmadan19
pushed a commit
to rishabhmadan19/llvm-project
that referenced
this pull request
Feb 9, 2026
When working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the `MessageHandler::OnClose`. Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle. This change moves the ownership of the `MainLoop::ReadHandleUP` and additionally own a reference to the `MainLoop` itself to ensure the loop outlives the JSONTransport object. This allows us to remove the handle immediately when we detect an EOF / hang up has occurred.
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jul 21, 2026
* [lldb] Refactor JSONTransport own MainLoop read handle. (llvm#179564) When working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the `MessageHandler::OnClose`. Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle. This change moves the ownership of the `MainLoop::ReadHandleUP` and additionally own a reference to the `MainLoop` itself to ensure the loop outlives the JSONTransport object. This allows us to remove the handle immediately when we detect an EOF / hang up has occurred. (cherry picked from commit 69878f9) * Fix use-after-free in IOTransport::OnRead on client disconnect (llvm#198548) When an MCP client disconnects (EOF), `IOTransport::OnRead` called `handler.OnClosed()` before resetting `m_read_handle`. The MCP server's `OnClosed` handler erases the client from `m_instances`, destroying both the transport (`this`) and the binder (`handler`). The subsequent `m_read_handle.reset()` then accessed the destroyed transport's member, causing a use-after-free (SIGSEGV). * thread #1, stop reason = signal SIGSEGV: address not mapped to object (fault address=0x28) * frame #0: 0x00007ff5d4d5afda liblldb.so.23.2`lldb_private::transport::IOTransport<lldb_protocol::mcp::ProtocolDescriptor>::OnRead(lldb_private::MainLoopBase&, lldb_private::transport::JSONTransport<lldb_protocol::mcp::ProtocolDescriptor>::MessageHandler&) + 1274 frame #1: 0x00007ff5d1140ad8 liblldb.so.23.0`lldb_private::MainLoopPosix::Run() + 408 frame #2: 0x00007ff5d1760c1c liblldb.so.23.0`std::thread::_State_impl<std::thre Fix by resetting the read handle before calling `OnClosed()`, so no transport members are accessed after the handler potentially destroys the transport. Then when the scope is left, the destructor is called for the new read_handle local variable and it is cleaned up. New unit tests added that fail without this change. With the change, the custom 'ai' script (allows end user locally to communicate lldb context to agent backend via a spun up MCP server: "protocol-server start MCP listen://localhost:{port}") now successfully concludes without this crash Assisted with: claude (cherry picked from commit 1e1f3dd) * [lldb] Add unit tests for the MCP server (llvm#202752) Add unit-test coverage for the MCP protocol types and server under source/Protocol/MCP and the MCP plugin under source/Plugins/Protocol/MCP. The Server handlers run over the in-memory TestTransport, which gains SimulateError/SimulateClosed/SetRegisterMessageHandlerShouldFail helpers to drive the handler lifecycle without a real socket. Code that touches the filesystem or otherwise requires mucking with the test environment are deliberately left uncovered until those layers can be mocked. Assisted-by: Claude (cherry picked from commit a85441c) * [lldb] Add an MCP client and asynchronous request binding (llvm#208371) This PR contains the groundwork to convert lldb-mcp from a naive forwarder into a multiplexer. This requires acting both an MCP server and MCP client. This PR adds a new MCP Client abstraction, a thin wrapper over MCPTransport and MCPBinder exposing the protocol's requests as typed asynchronous calls. BindAsync hands an incoming-request handler a Reply it may invoke later. This PR also makes fromJSON symmetric with toJSON for ServerCapabilities. The former only restored supportsToolsList and silently dropped the resources, completions and logging capabilities, so a client parsing an initialize result never saw them. Assisted-by: Claude (cherry picked from commit 15f7d14) * [lldb-mcp] Replace byte forwarding with a protocol-aware multiplexer (llvm#208506) To serve several LLDB instances behind one endpoint it has to act as a multiplexer. This PR adds a Multiplexer that presents a unified MCP server to the client. It answers initialize and tools/list locally, and forwards tools/call and the resource requests to the different instances through an mcp::Client (added in llvm#208371), relaying the answer back. For now, this still drives a single backend. Discovering and routing across several instances is coming next. Assisted-by: Claude (cherry picked from commit 5ddad72) * [lldb-mcp] Multiplex across all discovered LLDB instances (llvm#208827) Connect to every LLDB MCP server advertised under ~/.lldb rather than a single one, and present them to the client as one server. A stale registry entry from a crashed instance simply fails to connect and is skipped. Each instance is identified by the pid of its lldb process, now recorded in the ServerInfo registry file. Tools and resources are addressed with instance-qualified URIs, e.g. lldb-mcp://instance/{pid}/debugger/{id} and lldb://instance/{pid}/debugger/{id}/target/{idx}. Listing requests (sessions_list, resources/list) fan out to every backend and aggregate; targeted requests (command, resources/read) are routed by the pid parsed from the URI. Backends only know their local lldb-mcp://debugger/{id} form, so URIs are rewritten in both directions. Add Binder::FailPendingRequests (and Client::CancelPendingRequests) so that when the client disconnects with a request still in flight to a backend, the abandoned reply is satisfied with an error instead of being destroyed unanswered, which would trip the binder's "must reply" assert. The multiplexer cancels its backends on shutdown before unwinding. Assisted-by: Claude (cherry picked from commit d6d0ccc) * [lldb] Add MCP tools to create and destroy debugger instances (llvm#209288) Add debugger_create and debugger_delete tools to the MCP server so a client can manage debugger instances, not just command the ones that already exist. debugger_create detaches the new debugger's stdio from the host process (redirecting input/output/error to the null device) so its prompt and asynchronous output cannot corrupt an MCP stream that shares the host's stdout. Command results flow through CommandReturnObject and are unaffected. Factor the tool and resource registration out of ProtocolServerMCP::Extend into a shared PopulateServer() so an embedded in-process server (e.g. in lldb-mcp) can install the same set. Assisted-by: Claude rdar://181722721 (cherry picked from commit a6d35f4) * [lldb][bazel] Add the lldb-mcp binary to the Bazel overlay (llvm#209498) llvm#143628 added the lldb-mcp tool (an MCP server multiplexer for LLDB), built by lldb/tools/lldb-mcp/CMakeLists.txt. The Bazel overlay already models the ProtocolMCP library and the MCP protocol-server plugin, but not the lldb-mcp executable itself, so it is missing from the Bazel build. Add an lldb-mcp cc_binary modeled on the existing lldb-dap target: glob tools/lldb-mcp/**, expand the macOS Info.plist via expand_template, and depend on liblldb.wrapper, Host, Utility, ProtocolMCP, and the LLVM Option/Support libraries. lldb-mcp has no Options.td, so unlike lldb-dap no gentbl_cc_library is needed. This properly translates into BUCk internally at Meta and then builds with buck2, but I cannot validate the bazel build of this new target directly. bazel rule generation assisted with: claude (cherry picked from commit 543e714) * [lldb] Add SBProtocolServer to start protocol servers programmatically (llvm#209923) Starting a protocol server (such as MCP, and in the future potentially DAP) is only possible from the command line today, via `protocol-server start`. Add an SB API so an embedder can start one in its own process and learn where to connect. SBProtocolServer wraps ProtocolServer::GetOrCreate/Start/Stop and exposes the listening connection URI. This lets a tool host the engine's protocol server in-process and talk to it as a normal client, reusing the server's tools rather than reimplementing them. Assisted-by: Claude (cherry picked from commit 9b096bb) * [lldb-mcp] Host managed debug sessions in-process (llvm#210450) Let a client create and own debug sessions with session_create and session_close. Rather than spawn a separate lldb per session, lldb-mcp hosts them in its own process, communicating over a loopback socket to keep things uniform with external lldb instances. The benefits of this approach are: - There is no child-process machinery, so nothing needs to be spawned and cleaned up. - It works without the need for an external lldb binary. - It avoids the deadlock by reading stdin through a raw fd instead of the FILE* stdio path that previously hung the Debugger constructor contending on the REPL's stdin lock. - The architecture stays uniform between in-process and external sessions. The trade-off is no isolation, so an LLDB crash takes down lldb-mcp along with its proxied connections. Assisted-by: Claude (cherry picked from commit 9208fc3) --------- Co-authored-by: John Harrison <harjohn@google.com> Co-authored-by: youngd007 <davidayoung@meta.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When working with a MainLoop, if the file reaches the EOF it will immediately fire the read handle callback. We cannot readily determine if the file is at EOF or if the file is pointing to a socket/pipe that has consumed all the current data in the buffer but the remote end has not yet hung up. This is causing JSONTransport to continuously fire the OnRead callback trigging repeated calls to the
MessageHandler::OnClose.Since MainLoop does not perform the actual read, we need to adjust the behavior of JSONTransport to fully own the read handle.
This change moves the ownership of the
MainLoop::ReadHandleUPand additionally own a reference to theMainLoopitself to ensure the loop outlives the JSONTransport object.This allows us to remove the handle immediately when we detect an EOF / hang up has occurred.