Skip to content

Commit

Permalink
Simplify json rpc header parsing: remove lenient mode.
Browse files Browse the repository at this point in the history
We had a lenient mode for header parsing coming from the early
days of the implementation of the LSP protocol, however, that
is not used anymore. Remove it to simplify logic.
  • Loading branch information
hzeller committed Aug 1, 2023
1 parent 8f3b0e0 commit 6fd6e67
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 50 deletions.
5 changes: 1 addition & 4 deletions common/lsp/json-rpc-expect.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,7 @@ int main(int argc, char *argv[]) {
return usage(argv[0], "Input needs to be a json array");
}

// Let's be lenient in parsing.
const bool kStrictCRLFrequirement = true;
MessageStreamSplitter stream_splitter(4096, !kStrictCRLFrequirement);

MessageStreamSplitter stream_splitter(4096);
int first_error = -1;
size_t expect_pos = 0;
stream_splitter.SetMessageProcessor(
Expand Down
6 changes: 0 additions & 6 deletions common/lsp/message-stream-splitter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,11 @@ static constexpr int kGarbledHeader = -2;
int MessageStreamSplitter::ParseHeaderGetBodyOffset(absl::string_view data,
int *body_size) {
// TODO(hzeller): Make this more robust. Parse each \r\n section separately.
// Also: do we need lenient mode ?
static constexpr absl::string_view kEndHeaderMarker = "\r\n\r\n";
static constexpr absl::string_view kLenientEndHeaderMarker = "\n\n";
static constexpr absl::string_view kContentLengthHeader = "Content-Length: ";

int header_marker_len = kEndHeaderMarker.length();
auto end_of_header = data.find(kEndHeaderMarker);
if (end_of_header == absl::string_view::npos && lenient_lf_separation_) {
end_of_header = data.find(kLenientEndHeaderMarker);
header_marker_len = kLenientEndHeaderMarker.length();
}
if (end_of_header == absl::string_view::npos) return kIncompleteHeader;

// Very dirty search for header - we don't check if starts with line.
Expand Down
15 changes: 5 additions & 10 deletions common/lsp/message-stream-splitter.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,12 @@ class MessageStreamSplitter {
using MessageProcessFun =
std::function<void(absl::string_view header, absl::string_view body)>;

// Optional parameters are "initial_read_buffer_size" for the initial
// Optional parameter is "initial_read_buffer_size" for the initial
// internal buffer size (will be realloc'ed when needed).
// If "strict_crlf_header_separation" is false, also allows for simple
// newline as separation character in the header. Useful for manually
// speaking the protocol.
explicit MessageStreamSplitter(size_t initial_read_buffer_size = 4096,
bool strict_crlf_header_separation = true)
: read_buffer_(initial_read_buffer_size),
lenient_lf_separation_(!strict_crlf_header_separation) {}
explicit MessageStreamSplitter(size_t initial_read_buffer_size = 4096)
: read_buffer_(initial_read_buffer_size) {}
MessageStreamSplitter(const MessageStreamSplitter &) = delete;
MessageStreamSplitter(MessageStreamSplitter &&) = delete;

// Set the function that will receive extracted message bodies.
void SetMessageProcessor(const MessageProcessFun &message_processor) {
Expand Down Expand Up @@ -100,13 +96,12 @@ class MessageStreamSplitter {
absl::Status ReadInput(const ReadFun &read_fun);

std::vector<char> read_buffer_;
const bool lenient_lf_separation_;
absl::string_view pending_data_;

MessageProcessFun message_processor_;

size_t stats_largest_body_ = 0;
size_t stats_total_bytes_read_ = 0;
absl::string_view pending_data_;
};
} // namespace lsp
} // namespace verible
Expand Down
30 changes: 0 additions & 30 deletions common/lsp/message-stream-splitter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,36 +85,6 @@ TEST(MessageStreamSplitterTest, IgnoreHeadersNotNeeded) {
EXPECT_EQ(count_call, 7) << status;
}

TEST(MessageStreamSplitterTest, LenientHeaderNewline) {
for (const bool strict_header_crlf : {false, true}) {
MessageStreamSplitter header_test(256, strict_header_crlf);
int count_call = 0;
header_test.SetMessageProcessor(
[&count_call](absl::string_view, absl::string_view body) {
EXPECT_EQ(std::string(body), "x");
++count_call;
});
DataStreamSimulator two_messages(
"Content-Length: 1\r\n\r\nx" // this first one will always parse
"Content-Length: 1\n\nx"); // this will only parse if !strict

absl::Status status = absl::OkStatus();
while (status.ok()) {
status = header_test.PullFrom(
[&](char *buf, int size) { return two_messages.read(buf, size); });
}

if (strict_header_crlf) {
// The second message never contained \r\n\r\n, still pending at end.
EXPECT_EQ(status.code(), absl::StatusCode::kDataLoss) << status;
} else {
// An expected 'read everything reached EOF' status.
EXPECT_EQ(status.code(), absl::StatusCode::kUnavailable) << status;
}
EXPECT_EQ(count_call, strict_header_crlf ? 1 : 2) << status;
}
}

TEST(MessageStreamSplitterTest, CompleteReadValidMessage) {
static constexpr absl::string_view kHeader = "Content-Length: 3\r\n\r\n";
static constexpr absl::string_view kBody = "foo";
Expand Down

0 comments on commit 6fd6e67

Please sign in to comment.