Skip to content

[lldb] Update JSONTransport to use MainLoop for reading. #152367

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
merged 4 commits into from
Aug 12, 2025
Merged

[lldb] Update JSONTransport to use MainLoop for reading. #152367

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02417f5
[lldb] Update JSONTransport to use MainLoop for reading.
ashgti Aug 5, 2025
d5c1cd5
Merge remote-tracking branch 'origin/main' into lldb-json-transport
ashgti Aug 7, 2025
7143426
Moving away from [&] and explicitly copying/moving required lambda ca…
ashgti Aug 9, 2025
9b326a8
Adjusting the m_buffer SmallString size.
ashgti Aug 11, 2025
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
113 changes: 84 additions & 29 deletions lldb/include/lldb/Host/JSONTransport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
#ifndef LLDB_HOST_JSONTRANSPORT_H
#define LLDB_HOST_JSONTRANSPORT_H

#include "lldb/Host/MainLoopBase.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/JSON.h"
#include <chrono>
#include <string>
#include <system_error>
#include <vector>

namespace lldb_private {

Expand All @@ -28,27 +30,33 @@ class TransportEOFError : public llvm::ErrorInfo<TransportEOFError> {
static char ID;

TransportEOFError() = default;

void log(llvm::raw_ostream &OS) const override {
OS << "transport end of file reached";
}
void log(llvm::raw_ostream &OS) const override { OS << "transport EOF"; }
std::error_code convertToErrorCode() const override {
return llvm::inconvertibleErrorCode();
return std::make_error_code(std::errc::io_error);
}
};

class TransportTimeoutError : public llvm::ErrorInfo<TransportTimeoutError> {
class TransportUnhandledContentsError
: public llvm::ErrorInfo<TransportUnhandledContentsError> {
public:
static char ID;

TransportTimeoutError() = default;
explicit TransportUnhandledContentsError(std::string unhandled_contents)
: m_unhandled_contents(unhandled_contents) {}

void log(llvm::raw_ostream &OS) const override {
OS << "transport operation timed out";
OS << "transport EOF with unhandled contents " << m_unhandled_contents;
}
std::error_code convertToErrorCode() const override {
return std::make_error_code(std::errc::timed_out);
return std::make_error_code(std::errc::bad_message);
}

const std::string &getUnhandledContents() const {
return m_unhandled_contents;
}

private:
std::string m_unhandled_contents;
};

class TransportInvalidError : public llvm::ErrorInfo<TransportInvalidError> {
Expand All @@ -68,6 +76,10 @@ class TransportInvalidError : public llvm::ErrorInfo<TransportInvalidError> {
/// A transport class that uses JSON for communication.
class JSONTransport {
public:
using ReadHandleUP = MainLoopBase::ReadHandleUP;
template <typename T>
using Callback = std::function<void(MainLoopBase &, const llvm::Expected<T>)>;

JSONTransport(lldb::IOObjectSP input, lldb::IOObjectSP output);
virtual ~JSONTransport() = default;

Expand All @@ -83,24 +95,69 @@ class JSONTransport {
return WriteImpl(message);
}

/// Reads the next message from the input stream.
/// Registers the transport with the MainLoop.
template <typename T>
llvm::Expected<T> Read(const std::chrono::microseconds &timeout) {
llvm::Expected<std::string> message = ReadImpl(timeout);
if (!message)
return message.takeError();
return llvm::json::parse<T>(/*JSON=*/*message);
llvm::Expected<ReadHandleUP> RegisterReadObject(MainLoopBase &loop,
Callback<T> callback) {
Status error;
ReadHandleUP handle = loop.RegisterReadObject(
m_input,
[&](MainLoopBase &loop) {
Copy link
Collaborator

@labath labath Aug 8, 2025

Choose a reason for hiding this comment

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

I think the & is the problem, and making callback a std::function doesn't really solve it. Since this lambda outlives this function, it cannot capture any locals (or anything else that it outlives) by reference. This is why the Google style guide disallows default reference captures in long-lived lambdas. The LLVM style guide doesn't have an opinion on that, but I'd still recommend dropping the [&] and capturing the values you need explicitly. In particular, the callback needs to be captured by value -- which will also exclude the possibility of using unique_function for the callback (as it cannot be copied, but std::function is/can be).

We could also look into switching MainLoop to unique_function. I don't think it needs to copy the callback, and I wouldn't be surprised if its usage of std::function predates unique_function.

Copy link
Contributor Author

@ashgti ashgti Aug 9, 2025

Choose a reason for hiding this comment

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

Done. I removed the [&] usage and copied the values by value.

char buffer[kReadBufferSize];
size_t len = sizeof(buffer);
if (llvm::Error error = m_input->Read(buffer, len).takeError()) {
callback(loop, std::move(error));
return;
}

if (len)
m_buffer.append(std::string(buffer, len));

// If the buffer has contents, try parsing any pending messages.
if (!m_buffer.empty()) {
llvm::Expected<std::vector<std::string>> messages = Parse();
if (llvm::Error error = messages.takeError()) {
callback(loop, std::move(error));
return;
}

for (const auto &message : *messages)
if constexpr (std::is_same<T, std::string>::value)
callback(loop, message);
else
callback(loop, llvm::json::parse<T>(message));
}

// On EOF, notify the callback after the remaining messages were
// handled.
if (len == 0) {
if (m_buffer.empty())
callback(loop, llvm::make_error<TransportEOFError>());
else
callback(loop, llvm::make_error<TransportUnhandledContentsError>(
m_buffer));
}
},
error);
if (error.Fail())
return error.takeError();
return handle;
}

protected:
template <typename... Ts> inline auto Logv(const char *Fmt, Ts &&...Vals) {
Log(llvm::formatv(Fmt, std::forward<Ts>(Vals)...).str());
}
virtual void Log(llvm::StringRef message);

virtual llvm::Error WriteImpl(const std::string &message) = 0;
virtual llvm::Expected<std::string>
ReadImpl(const std::chrono::microseconds &timeout) = 0;
virtual llvm::Expected<std::vector<std::string>> Parse() = 0;

lldb::IOObjectSP m_input;
lldb::IOObjectSP m_output;
std::string m_buffer;

static constexpr size_t kReadBufferSize = 1024;
};

/// A transport class for JSON with a HTTP header.
Expand All @@ -111,14 +168,13 @@ class HTTPDelimitedJSONTransport : public JSONTransport {
virtual ~HTTPDelimitedJSONTransport() = default;

protected:
virtual llvm::Error WriteImpl(const std::string &message) override;
virtual llvm::Expected<std::string>
ReadImpl(const std::chrono::microseconds &timeout) override;

// FIXME: Support any header.
static constexpr llvm::StringLiteral kHeaderContentLength =
"Content-Length: ";
static constexpr llvm::StringLiteral kHeaderSeparator = "\r\n\r\n";
llvm::Error WriteImpl(const std::string &message) override;
llvm::Expected<std::vector<std::string>> Parse() override;

static constexpr llvm::StringLiteral kHeaderContentLength = "Content-Length";
static constexpr llvm::StringLiteral kHeaderFieldSeparator = ":";
static constexpr llvm::StringLiteral kHeaderSeparator = "\r\n";
static constexpr llvm::StringLiteral kEndOfHeader = "\r\n\r\n";
};

/// A transport class for JSON RPC.
Expand All @@ -129,9 +185,8 @@ class JSONRPCTransport : public JSONTransport {
virtual ~JSONRPCTransport() = default;

protected:
virtual llvm::Error WriteImpl(const std::string &message) override;
virtual llvm::Expected<std::string>
ReadImpl(const std::chrono::microseconds &timeout) override;
llvm::Error WriteImpl(const std::string &message) override;
llvm::Expected<std::vector<std::string>> Parse() override;

static constexpr llvm::StringLiteral kMessageSeparator = "\n";
};
Expand Down
Loading
Loading