Addressing reviewer feedback.

ashgti · ashgti · commit e43e1a63e4a9 · 2025-07-30T09:18:05.000-07:00
diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h
@@ -30,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> {
@@ -97,19 +103,14 @@ class JSONTransport {
     ReadHandleUP handle = loop.RegisterReadObject(
         m_input,
         [&](MainLoopBase &loop) {
-          char buf[1024];
-          size_t len = sizeof(buf);
-          do {
-            if (llvm::Error error = m_input->Read(buf, len).takeError()) {
-              callback(loop, std::move(error));
-              return;
-            }
-
-            if (len == 0) // EOF
-              break;
+          char buffer[kReadBufferSize];
+          size_t len = sizeof(buffer);
+          if (llvm::Error error = m_input->Read(buffer, len).takeError()) {
+            callback(loop, std::move(error));
+            return;
+          }
 
-            m_buffer.append(std::string(buf, len));
-          } while (len == sizeof(buf));
+          m_buffer.append(std::string(buffer, len));
 
           llvm::Expected<std::vector<std::string>> messages = Parse();
           if (llvm::Error error = messages.takeError()) {
@@ -125,8 +126,13 @@ class JSONTransport {
 
           // On EOF, notify the callback after the remaining messages were
           // handled.
-          if (len == 0)
-            callback(loop, llvm::make_error<TransportEOFError>());
+          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())
@@ -135,6 +141,9 @@ class JSONTransport {
   }
 
 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;
@@ -143,6 +152,8 @@ class JSONTransport {
   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.
diff --git a/lldb/source/Host/common/JSONTransport.cpp b/lldb/source/Host/common/JSONTransport.cpp
@@ -29,56 +29,58 @@ void JSONTransport::Log(llvm::StringRef message) {
   LLDB_LOG(GetLog(LLDBLog::Host), "{0}", message);
 }
 
+// Parses messages based on
+// https://microsoft.github.io/debug-adapter-protocol/overview#base-protocol
 Expected<std::vector<std::string>> HTTPDelimitedJSONTransport::Parse() {
   if (m_buffer.empty())
     return std::vector<std::string>{};
 
   std::vector<std::string> messages;
-  llvm::StringRef buf = m_buffer;
+  StringRef buffer = m_buffer;
   size_t content_length = 0, end_of_last_message = 0, cursor = 0;
   do {
-    auto idx = buf.find(kHeaderSeparator, cursor);
+    auto idx = buffer.find(kHeaderSeparator, cursor);
+    // Separator not found, we need more data.
     if (idx == StringRef::npos)
       break;
 
-    auto header = buf.slice(cursor, idx);
+    auto header = buffer.slice(cursor, idx);
     cursor = idx + kHeaderSeparator.size();
 
     // An empty line separates the headers from the message body.
     if (header.empty()) {
-      // Not enough data, wait for the next chunk to arrive.
-      if (content_length + cursor > buf.size())
+      // Check if we have enough data or wait for the next chunk to arrive.
+      if (content_length + cursor > buffer.size())
         break;
 
-      std::string body = buf.substr(cursor, content_length).str();
+      std::string body = buffer.substr(cursor, content_length).str();
       end_of_last_message = cursor + content_length;
       cursor += content_length;
-      Log(llvm::formatv("--> {0}", body).str());
-      messages.push_back(body);
+      Logv("--> {0}", body);
+      messages.emplace_back(std::move(body));
       content_length = 0;
       continue;
     }
 
     // HTTP Headers are formatted like `<field-name> ':' [<field-value>]`.
     if (!header.contains(kHeaderFieldSeparator))
-      return make_error<StringError>("malformed content header",
-                                     inconvertibleErrorCode());
+      return createStringError("malformed content header");
 
     auto [name, value] = header.split(kHeaderFieldSeparator);
 
-    // Handle known headers, at the moment only "Content-Length" is supported,
+    // Handle known headers, at the moment only "Content-Length" is specified,
     // other headers are ignored.
     if (name.lower() == kHeaderContentLength.lower()) {
       value = value.trim();
       if (value.trim().consumeInteger(10, content_length))
-        return make_error<StringError>(
-            formatv("invalid content length: {0}", value).str(),
-            inconvertibleErrorCode());
+        return createStringError(std::errc::invalid_argument,
+                                 "invalid content length: %s",
+                                 value.str().c_str());
     }
-  } while (cursor < buf.size());
+  } while (cursor < buffer.size());
 
   // Store the remainder of the buffer for the next read callback.
-  m_buffer = buf.substr(end_of_last_message);
+  m_buffer = buffer.substr(end_of_last_message);
 
   return messages;
 }
@@ -87,7 +89,7 @@ Error HTTPDelimitedJSONTransport::WriteImpl(const std::string &message) {
   if (!m_output || !m_output->IsValid())
     return llvm::make_error<TransportInvalidError>();
 
-  Log(llvm::formatv("<-- {0}", message).str());
+  Logv("<-- {0}", message);
 
   std::string Output;
   raw_string_ostream OS(Output);
@@ -106,7 +108,7 @@ Expected<std::vector<std::string>> JSONRPCTransport::Parse() {
       break;
     std::string raw_json = buf.substr(0, idx).str();
     buf = buf.substr(idx + 1);
-    Log(llvm::formatv("--> {0}", raw_json).str());
+    Logv("--> {0}", raw_json);
     messages.push_back(raw_json);
   } while (!buf.empty());
 
@@ -120,7 +122,7 @@ Error JSONRPCTransport::WriteImpl(const std::string &message) {
   if (!m_output || !m_output->IsValid())
     return llvm::make_error<TransportInvalidError>();
 
-  Log(llvm::formatv("<-- {0}", message).str());
+  Logv("<-- {0}", message);
 
   std::string Output;
   llvm::raw_string_ostream OS(Output);
@@ -130,5 +132,5 @@ Error JSONRPCTransport::WriteImpl(const std::string &message) {
 }
 
 char TransportEOFError::ID;
-char TransportTimeoutError::ID;
+char TransportUnhandledContentsError::ID;
 char TransportInvalidError::ID;
diff --git a/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py b/lldb/test/API/tools/lldb-dap/io/TestDAP_io.py
@@ -8,6 +8,9 @@
 import lldbdap_testcase
 import dap_server
 
+EXIT_FAILURE = 1
+EXIT_SUCCESS = 0
+
 
 class TestDAP_io(lldbdap_testcase.DAPTestCaseBase):
     def launch(self):
@@ -41,40 +44,44 @@ def test_eof_immediately(self):
         """
         process = self.launch()
         process.stdin.close()
-        self.assertEqual(process.wait(timeout=5.0), 0)
+        self.assertEqual(process.wait(timeout=5.0), EXIT_SUCCESS)
 
     def test_invalid_header(self):
         """
-        lldb-dap handles invalid message headers.
+        lldb-dap returns a failure exit code when the input stream is closed
+        with a malformed request header.
         """
         process = self.launch()
         process.stdin.write(b"not the correct message header")
         process.stdin.close()
-        self.assertEqual(process.wait(timeout=5.0), 0)
+        self.assertEqual(process.wait(timeout=5.0), EXIT_FAILURE)
 
     def test_partial_header(self):
         """
-        lldb-dap handles partial message headers.
+        lldb-dap returns a failure exit code when the input stream is closed
+        with an incomplete message header is in the message buffer.
         """
         process = self.launch()
         process.stdin.write(b"Content-Length: ")
         process.stdin.close()
-        self.assertEqual(process.wait(timeout=5.0), 0)
+        self.assertEqual(process.wait(timeout=5.0), EXIT_FAILURE)
 
     def test_incorrect_content_length(self):
         """
-        lldb-dap handles malformed content length headers.
+        lldb-dap returns a failure exit code when reading malformed content
+        length headers.
         """
         process = self.launch()
         process.stdin.write(b"Content-Length: abc")
         process.stdin.close()
-        self.assertEqual(process.wait(timeout=5.0), 0)
+        self.assertEqual(process.wait(timeout=5.0), EXIT_FAILURE)
 
     def test_partial_content_length(self):
         """
-        lldb-dap handles partial messages.
+        lldb-dap returns a failure exit code when the input stream is closed
+        with a partial message in the message buffer.
         """
         process = self.launch()
         process.stdin.write(b"Content-Length: 10\r\n\r\n{")
         process.stdin.close()
-        self.assertEqual(process.wait(timeout=5.0), 0)
+        self.assertEqual(process.wait(timeout=5.0), EXIT_FAILURE)
diff --git a/lldb/unittests/Host/JSONTransportTest.cpp b/lldb/unittests/Host/JSONTransportTest.cpp
@@ -146,6 +146,21 @@ TEST_F(HTTPDelimitedJSONTransportTest, ReadWithEOF) {
   });
 }
 
+TEST_F(HTTPDelimitedJSONTransportTest, ReaderWithUnhandledData) {
+  std::string json = R"json({"str": "foo"})json";
+  std::string message =
+      formatv("Content-Length: {0}\r\nContent-type: text/json\r\n\r\n{1}",
+              json.size(), json)
+          .str();
+  // Write an incomplete message and close the handle.
+  ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size() - 1),
+                       Succeeded());
+  input.CloseWriteFileDescriptor();
+  RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
+    ASSERT_THAT_EXPECTED(message, Failed<TransportUnhandledContentsError>());
+  });
+}
+
 TEST_F(HTTPDelimitedJSONTransportTest, InvalidTransport) {
   transport = std::make_unique<HTTPDelimitedJSONTransport>(nullptr, nullptr);
   auto handle = transport->RegisterReadObject<JSONTestType>(
@@ -219,6 +234,18 @@ TEST_F(JSONRPCTransportTest, ReadWithEOF) {
   });
 }
 
+TEST_F(JSONRPCTransportTest, ReaderWithUnhandledData) {
+  std::string message = R"json({"str": "foo"})json"
+                        "\n";
+  // Write an incomplete message and close the handle.
+  ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size() - 1),
+                       Succeeded());
+  input.CloseWriteFileDescriptor();
+  RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
+    ASSERT_THAT_EXPECTED(message, Failed<TransportUnhandledContentsError>());
+  });
+}
+
 TEST_F(JSONRPCTransportTest, Write) {
   ASSERT_THAT_ERROR(transport->Write(JSONTestType{"foo"}), Succeeded());
   output.CloseWriteFileDescriptor();
