Skip to content

[lldb] Refactor JSONTransport own MainLoop read handle. #179564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashgti merged 2 commits into from
Feb 6, 2026
Merged

[lldb] Refactor JSONTransport own MainLoop read handle. #179564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
483dad5
[lldb] Refactor JSONTransport own MainLoop read handle.
ashgti Feb 3, 2026
0e5d011
Trimming an unused parameter and fixing up some headers for files tha…
ashgti Feb 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions lldb/include/lldb/Host/JSONTransport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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();
}

Comment on lines +184 to 206

@da-viper da-viper Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ashgti ashgti Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@da-viper da-viper Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ashgti ashgti Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

/// Public for testing purposes, otherwise this should be an implementation
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions lldb/include/lldb/Protocol/MCP/Server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/JSON.h"
#include "llvm/Support/Signals.h"
#include <map>
#include <memory>
#include <string>
#include <vector>
Expand All @@ -40,7 +41,7 @@ class Server {
void AddTool(std::unique_ptr<Tool> tool);
void AddResourceProvider(std::unique_ptr<ResourceProvider> resource_provider);

llvm::Error Accept(lldb_private::MainLoop &, MCPTransportUP);
llvm::Error Accept(MCPTransportUP);

protected:
MCPBinderUP Bind(MCPTransport &);
Expand Down Expand Up @@ -70,7 +71,6 @@ class Server {

LogCallback m_log_callback;
struct Client {
ReadHandleUP handle;
MCPTransportUP transport;
MCPBinderUP binder;
};
Expand Down
4 changes: 2 additions & 2 deletions lldb/include/lldb/Protocol/MCP/Transport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ 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);
});

if (auto error = m_server->Accept(m_loop, std::move(transport_up)))
if (auto error = m_server->Accept(std::move(transport_up)))
LLDB_LOG_ERROR(log, std::move(error), "{0}:");
}

Expand Down
7 changes: 4 additions & 3 deletions lldb/source/Plugins/Protocol/MCP/ProtocolServerMCP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
#include "lldb/Host/MainLoop.h"
#include "lldb/Host/Socket.h"
#include "lldb/Protocol/MCP/Server.h"
#include "lldb/Protocol/MCP/Transport.h"
#include <map>
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include <cstddef>
#include <memory>
#include <mutex>
#include <thread>
#include <tuple>
#include <vector>

namespace lldb_private::mcp {
Expand Down
10 changes: 4 additions & 6 deletions lldb/source/Protocol/MCP/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ MCPBinderUP Server::Bind(MCPTransport &transport) {
return binder_up;
}

llvm::Error Server::Accept(MainLoop &loop, MCPTransportUP transport) {
llvm::Error Server::Accept(MCPTransportUP transport) {
MCPBinderUP binder = Bind(*transport);
MCPTransport *transport_ptr = transport.get();
binder->OnDisconnect([this, transport_ptr]() {
Expand All @@ -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();
}

Expand Down
7 changes: 4 additions & 3 deletions lldb/source/Protocol/MCP/Transport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 2 additions & 3 deletions lldb/tools/lldb-dap/DAP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions lldb/tools/lldb-dap/Transport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions lldb/tools/lldb-dap/Transport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 2 additions & 3 deletions lldb/tools/lldb-dap/tool/lldb-dap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down
11 changes: 3 additions & 8 deletions lldb/unittests/DAP/TestBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand All @@ -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() {
Expand Down
1 change: 0 additions & 1 deletion lldb/unittests/DAP/TestBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
40 changes: 19 additions & 21 deletions lldb/unittests/Host/JSONTransportTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
}

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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."));
}

Expand Down Expand Up @@ -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."));
}

Expand Down
10 changes: 3 additions & 7 deletions lldb/unittests/Protocol/ProtocolMCPServerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Loading
Loading