llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> The transport dropped a message that failed to parse: it logged the error and returned, so a peer waiting on that request hung forever and every message already buffered behind it was discarded. Keep going after a failed parse, and add ReplyWithParseError so a message that is valid JSON but not a valid request is answered against its own id. Unparseable JSON still gets no reply, since it carries no id and JSON-RPC forbids inventing one. Method-not-found was raised with createStringError, which converts through inconvertibleErrorCode() to -32603, so clients could not tell a misspelled method from a server fault. Use the MethodNotFound error, which already carries -32601, and give InvalidParams the -32602 code it was missing. --- Full diff: https://github.com/llvm/llvm-project/pull/212678.diff 5 Files Affected: - (modified) lldb/include/lldb/Host/JSONTransport.h (+41-4) - (modified) lldb/include/lldb/Protocol/MCP/Transport.h (+3) - (modified) lldb/source/Host/common/JSONTransport.cpp (+13-1) - (modified) lldb/source/Protocol/MCP/Transport.cpp (+29) - (modified) lldb/unittests/Host/JSONTransportTest.cpp (+21) ``````````diff diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h index d3c6e8e101ad5..a6170303e13f0 100644 --- a/lldb/include/lldb/Host/JSONTransport.h +++ b/lldb/include/lldb/Host/JSONTransport.h @@ -68,6 +68,8 @@ class InvalidParams : public llvm::ErrorInfo<InvalidParams> { public: static char ID; + static constexpr int kErrorCode = -32602; + explicit InvalidParams(std::string method, std::string context) : m_method(std::move(method)), m_context(std::move(context)) {} @@ -83,6 +85,25 @@ class InvalidParams : public llvm::ErrorInfo<InvalidParams> { std::string m_context; }; +/// An error to indicate that an incoming message could not be parsed as a +/// valid protocol message. +class InvalidMessage : public llvm::ErrorInfo<InvalidMessage> { +public: + static char ID; + + static constexpr int kErrorCode = -32700; + + explicit InvalidMessage(std::string raw_message, std::string reason) + : m_raw_message(std::move(raw_message)), m_reason(std::move(reason)) {} + + void log(llvm::raw_ostream &OS) const override; + std::error_code convertToErrorCode() const override; + +private: + std::string m_raw_message; + std::string m_reason; +}; + /// An error to indicate that no handler was registered for a given method. class MethodNotFound : public llvm::ErrorInfo<MethodNotFound> { public: @@ -139,6 +160,14 @@ class JSONTransport { /// Sends a response to a specific request. virtual llvm::Error Send(const Resp &) = 0; + /// Sends an error response for a message that failed to parse, described by + /// \p reason. Sends nothing if no request id can be recovered from it, since + /// there is then no request to respond to. + virtual llvm::Error ReplyWithParseError(llvm::StringRef raw_message, + llvm::StringRef reason) { + return llvm::Error::success(); + } + /// Implemented to handle incoming messages. (See `RegisterMessageHandler()` /// below). class MessageHandler { @@ -245,8 +274,15 @@ template <typename Proto> class IOTransport : public JSONTransport<Proto> { llvm::Expected<Message> message = llvm::json::parse<Message>(raw_message); if (!message) { - handler.OnError(message.takeError()); - return; + // Messages are independent, so one that fails to parse must not + // discard those already buffered behind it. + std::string reason = llvm::toString(message.takeError()); + if (llvm::Error error = + this->ReplyWithParseError(raw_message, reason)) + handler.OnError(std::move(error)); + handler.OnError( + llvm::make_error<InvalidMessage>(raw_message, std::move(reason))); + continue; } std::visit([&handler](auto &&msg) { handler.Received(msg); }, *message); @@ -546,7 +582,7 @@ class Binder : public JSONTransport<Proto>::MessageHandler { auto it = m_event_handlers.find(Proto::KeyFor(evt)); if (it == m_event_handlers.end()) { OnError(llvm::createStringError( - llvm::formatv("no handled for event {0}", toJSON(evt)))); + llvm::formatv("no handler for event {0}", toJSON(evt)))); return; } it->second(evt); @@ -558,7 +594,8 @@ class Binder : public JSONTransport<Proto>::MessageHandler { std::scoped_lock<std::recursive_mutex> guard(m_mutex); auto it = m_request_handlers.find(Proto::KeyFor(req)); if (it == m_request_handlers.end()) { - reply(Proto::Make(req, llvm::createStringError("method not found"))); + reply(Proto::Make(req, + llvm::make_error<MethodNotFound>(Proto::KeyFor(req)))); return; } diff --git a/lldb/include/lldb/Protocol/MCP/Transport.h b/lldb/include/lldb/Protocol/MCP/Transport.h index ceadf1dbd82b8..4e55b7939cfe6 100644 --- a/lldb/include/lldb/Protocol/MCP/Transport.h +++ b/lldb/include/lldb/Protocol/MCP/Transport.h @@ -95,6 +95,9 @@ class Transport final void Log(llvm::StringRef message) override; + llvm::Error ReplyWithParseError(llvm::StringRef raw_message, + llvm::StringRef reason) override; + private: LogCallback m_log_callback; }; diff --git a/lldb/source/Host/common/JSONTransport.cpp b/lldb/source/Host/common/JSONTransport.cpp index 22de7fa8cbead..17af066dfa80d 100644 --- a/lldb/source/Host/common/JSONTransport.cpp +++ b/lldb/source/Host/common/JSONTransport.cpp @@ -37,7 +37,19 @@ void InvalidParams::log(raw_ostream &OS) const { << "'"; } std::error_code InvalidParams::convertToErrorCode() const { - return std::make_error_code(std::errc::invalid_argument); + // JSON-RPC Invalid params + return std::error_code(InvalidParams::kErrorCode, std::generic_category()); +} + +char InvalidMessage::ID; + +void InvalidMessage::log(raw_ostream &OS) const { + OS << "invalid message '" << m_raw_message << "': '" << m_reason << "'"; +} + +std::error_code InvalidMessage::convertToErrorCode() const { + // JSON-RPC Parse error + return std::error_code(InvalidMessage::kErrorCode, std::generic_category()); } char MethodNotFound::ID; diff --git a/lldb/source/Protocol/MCP/Transport.cpp b/lldb/source/Protocol/MCP/Transport.cpp index 1dc01a9f59008..47a3ff2b6f77c 100644 --- a/lldb/source/Protocol/MCP/Transport.cpp +++ b/lldb/source/Protocol/MCP/Transport.cpp @@ -22,3 +22,32 @@ void Transport::Log(StringRef message) { if (m_log_callback) m_log_callback(message); } + +llvm::Error Transport::ReplyWithParseError(StringRef raw_message, + StringRef reason) { + llvm::Expected<json::Value> value = json::parse(raw_message); + if (!value) { + // JSON-RPC forbids guessing an id, and malformed JSON carries none. + consumeError(value.takeError()); + return llvm::Error::success(); + } + + const json::Object *object = value->getAsObject(); + if (!object) + return llvm::Error::success(); + + // A message without an id is a notification, which takes no response. + const json::Value *raw_id = object->get("id"); + if (!raw_id) + return llvm::Error::success(); + + Id id; + if (std::optional<StringRef> str = raw_id->getAsString()) + id = str->str(); + else if (std::optional<int64_t> num = raw_id->getAsInteger()) + id = *num; + else + return llvm::Error::success(); + + return Send(Response{id, mcp::Error{eErrorCodeInvalidRequest, reason.str()}}); +} diff --git a/lldb/unittests/Host/JSONTransportTest.cpp b/lldb/unittests/Host/JSONTransportTest.cpp index c0adbd855cec8..d54576b9af900 100644 --- a/lldb/unittests/Host/JSONTransportTest.cpp +++ b/lldb/unittests/Host/JSONTransportTest.cpp @@ -546,6 +546,19 @@ TEST_F(JSONRPCTransportTest, MalformedRequests) { ASSERT_THAT_ERROR(Run(), Succeeded()); } +TEST_F(JSONRPCTransportTest, MalformedRequestDoesNotDropLaterMessages) { + InSequence seq; + std::string messages = + "notjson\n" + Encode(Message{Request{1, "foo", std::nullopt}}); + ASSERT_THAT_EXPECTED(input.Write(messages.data(), messages.size()), + Succeeded()); + EXPECT_CALL(message_handler, OnError(_)).WillOnce([](llvm::Error err) { + consumeError(std::move(err)); + }); + EXPECT_CALL(message_handler, Received(Request{1, "foo", std::nullopt})); + ASSERT_THAT_ERROR(Run(), Succeeded()); +} + TEST_F(JSONRPCTransportTest, Read) { Write(Message{Request{1, "foo", std::nullopt}}); EXPECT_CALL(message_handler, Received(Request{1, "foo", std::nullopt})); @@ -831,6 +844,14 @@ TEST_F(TransportBinderTest, InBoundAsyncRequestsError) { Run(); } +TEST_F(TransportBinderTest, InBoundRequestUnknownMethod) { + EXPECT_THAT_ERROR(from_remote->Send(Request{4, "nosuch", MyFnParams{1, 2}}), + Succeeded()); + EXPECT_CALL(remote, Received(Response{4, MethodNotFound::kErrorCode, + "method not found: 'nosuch'"})); + Run(); +} + TEST_F(TransportBinderTest, FailPendingRequests) { OutgoingRequest<MyFnResult, MyFnParams> addFn = binder->Bind<MyFnResult, MyFnParams>("add"); `````````` </details> https://github.com/llvm/llvm-project/pull/212678 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
