Skip to content

Commit d6e44d7

Browse files
ashgtiashgti
committed
Updating HTTPDelimitedJSONTransport::Parse to make it more readable.
1 parent e43e1a6 commit d6e44d7

File tree

3 files changed

+100
-94
lines changed

3 files changed

+100
-94
lines changed

‎lldb/include/lldb/Host/JSONTransport.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class JSONTransport {
7878
public:
7979
using ReadHandleUP = MainLoopBase::ReadHandleUP;
8080
template <typename T>
81-
using Callback = std::function<void(MainLoopBase &, llvm::Expected<T>)>;
81+
using Callback = std::function<void(MainLoopBase &, const llvm::Expected<T>)>;
8282

8383
JSONTransport(lldb::IOObjectSP input, lldb::IOObjectSP output);
8484
virtual ~JSONTransport() = default;
@@ -98,7 +98,7 @@ class JSONTransport {
9898
/// Registers the transport with the MainLoop.
9999
template <typename T>
100100
llvm::Expected<ReadHandleUP> RegisterReadObject(MainLoopBase &loop,
101-
Callback<T> callback) {
101+
const Callback<T> &callback) {
102102
Status error;
103103
ReadHandleUP handle = loop.RegisterReadObject(
104104
m_input,
@@ -170,6 +170,7 @@ class HTTPDelimitedJSONTransport : public JSONTransport {
170170
static constexpr llvm::StringLiteral kHeaderContentLength = "Content-Length";
171171
static constexpr llvm::StringLiteral kHeaderFieldSeparator = ":";
172172
static constexpr llvm::StringLiteral kHeaderSeparator = "\r\n";
173+
static constexpr llvm::StringLiteral kEndOfHeader = "\r\n\r\n";
173174
};
174175

175176
/// A transport class for JSON RPC.

‎lldb/source/Host/common/JSONTransport.cpp‎

Lines changed: 29 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -32,57 +32,42 @@ void JSONTransport::Log(llvm::StringRef message) {
3232
// Parses messages based on
3333
// https://microsoft.github.io/debug-adapter-protocol/overview#base-protocol
3434
Expected<std::vector<std::string>> HTTPDelimitedJSONTransport::Parse() {
35-
if (m_buffer.empty())
36-
return std::vector<std::string>{};
37-
3835
std::vector<std::string> messages;
3936
StringRef buffer = m_buffer;
40-
size_t content_length = 0, end_of_last_message = 0, cursor = 0;
41-
do {
42-
auto idx = buffer.find(kHeaderSeparator, cursor);
43-
// Separator not found, we need more data.
44-
if (idx == StringRef::npos)
45-
break;
46-
47-
auto header = buffer.slice(cursor, idx);
48-
cursor = idx + kHeaderSeparator.size();
49-
50-
// An empty line separates the headers from the message body.
51-
if (header.empty()) {
52-
// Check if we have enough data or wait for the next chunk to arrive.
53-
if (content_length + cursor > buffer.size())
54-
break;
55-
56-
std::string body = buffer.substr(cursor, content_length).str();
57-
end_of_last_message = cursor + content_length;
58-
cursor += content_length;
59-
Logv("--> {0}", body);
60-
messages.emplace_back(std::move(body));
61-
content_length = 0;
62-
continue;
63-
}
64-
37+
while (buffer.contains(kEndOfHeader)) {
38+
auto [headers, rest] = buffer.split(kEndOfHeader);
39+
SmallVector<StringRef> kv_pairs;
6540
// HTTP Headers are formatted like `<field-name> ':' [<field-value>]`.
66-
if (!header.contains(kHeaderFieldSeparator))
67-
return createStringError("malformed content header");
68-
69-
auto [name, value] = header.split(kHeaderFieldSeparator);
41+
headers.split(kv_pairs, kHeaderSeparator);
42+
size_t content_length = 0;
43+
for (const auto &header : kv_pairs) {
44+
auto [key, value] = header.split(kHeaderFieldSeparator);
45+
// 'Content-Length' is the only meaningful key at the moment. Others are
46+
// ignored.
47+
if (!key.equals_insensitive(kHeaderContentLength))
48+
continue;
7049

71-
// Handle known headers, at the moment only "Content-Length" is specified,
72-
// other headers are ignored.
73-
if (name.lower() == kHeaderContentLength.lower()) {
7450
value = value.trim();
75-
if (value.trim().consumeInteger(10, content_length))
51+
if (!llvm::to_integer(value, content_length, 10))
7652
return createStringError(std::errc::invalid_argument,
7753
"invalid content length: %s",
7854
value.str().c_str());
7955
}
80-
} while (cursor < buffer.size());
56+
57+
// Check if we have enough data.
58+
if (content_length > rest.size())
59+
break;
60+
61+
StringRef body = rest.take_front(content_length);
62+
buffer = rest.drop_front(content_length);
63+
messages.emplace_back(body.str());
64+
Logv("--> {0}", body);
65+
}
8166

8267
// Store the remainder of the buffer for the next read callback.
83-
m_buffer = buffer.substr(end_of_last_message);
68+
m_buffer = buffer.str();
8469

85-
return messages;
70+
return std::move(messages);
8671
}
8772

8873
Error HTTPDelimitedJSONTransport::WriteImpl(const std::string &message) {
@@ -102,15 +87,12 @@ Error HTTPDelimitedJSONTransport::WriteImpl(const std::string &message) {
10287
Expected<std::vector<std::string>> JSONRPCTransport::Parse() {
10388
std::vector<std::string> messages;
10489
StringRef buf = m_buffer;
105-
do {
106-
size_t idx = buf.find(kMessageSeparator);
107-
if (idx == StringRef::npos)
108-
break;
109-
std::string raw_json = buf.substr(0, idx).str();
110-
buf = buf.substr(idx + 1);
90+
while (buf.contains(kMessageSeparator)) {
91+
auto [raw_json, rest] = buf.split(kMessageSeparator);
92+
buf = rest;
93+
messages.emplace_back(raw_json.str());
11194
Logv("--> {0}", raw_json);
112-
messages.push_back(raw_json);
113-
} while (!buf.empty());
95+
}
11496

11597
// Store the remainder of the buffer for the next read callback.
11698
m_buffer = buf.str();

‎lldb/unittests/Host/JSONTransportTest.cpp‎

Lines changed: 68 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -39,22 +39,37 @@ template <typename T> class JSONTransportTest : public PipePairTest {
3939
}
4040

4141
template <typename P>
42-
void
43-
RunOnce(std::function<void(llvm::Expected<P>)> callback,
44-
std::chrono::milliseconds timeout = std::chrono::milliseconds(100)) {
42+
Expected<P>
43+
RunOnce(std::chrono::milliseconds timeout = std::chrono::seconds(1)) {
44+
std::promise<Expected<P>> promised_message;
45+
RunUntil<P>(
46+
[&](Expected<P> message) {
47+
promised_message.set_value(std::move(message));
48+
return /*keep_going*/ false;
49+
},
50+
timeout);
51+
return promised_message.get_future().get();
52+
}
53+
54+
/// RunUntil runs the event loop until the callback returns `false` or a
55+
/// timeout has occured.
56+
template <typename P>
57+
void RunUntil(std::function<bool(Expected<P>)> callback,
58+
std::chrono::milliseconds timeout = std::chrono::seconds(1)) {
4559
auto handle = transport->RegisterReadObject<P>(
4660
loop, [&](MainLoopBase &loop, llvm::Expected<P> message) {
47-
callback(std::move(message));
48-
loop.RequestTermination();
61+
bool keep_going = callback(std::move(message));
62+
if (!keep_going)
63+
loop.RequestTermination();
4964
});
5065
loop.AddCallback(
5166
[&](MainLoopBase &loop) {
5267
loop.RequestTermination();
53-
FAIL() << "timeout waiting for read callback";
68+
callback(createStringError("timeout"));
5469
},
5570
timeout);
56-
ASSERT_THAT_EXPECTED(handle, Succeeded());
57-
ASSERT_THAT_ERROR(loop.Run().takeError(), Succeeded());
71+
EXPECT_THAT_EXPECTED(handle, Succeeded());
72+
EXPECT_THAT_ERROR(loop.Run().takeError(), Succeeded());
5873
}
5974
};
6075

@@ -89,10 +104,8 @@ TEST_F(HTTPDelimitedJSONTransportTest, MalformedRequests) {
89104
ASSERT_THAT_EXPECTED(
90105
input.Write(malformed_header.data(), malformed_header.size()),
91106
Succeeded());
92-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
93-
ASSERT_THAT_EXPECTED(message,
94-
FailedWithMessage("invalid content length: -1"));
95-
});
107+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
108+
FailedWithMessage("invalid content length: -1"));
96109
}
97110

98111
TEST_F(HTTPDelimitedJSONTransportTest, Read) {
@@ -103,8 +116,32 @@ TEST_F(HTTPDelimitedJSONTransportTest, Read) {
103116
.str();
104117
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
105118
Succeeded());
106-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
107-
ASSERT_THAT_EXPECTED(message, HasValue(testing::FieldsAre(/*str=*/"foo")));
119+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
120+
HasValue(testing::FieldsAre(/*str=*/"foo")));
121+
}
122+
123+
TEST_F(HTTPDelimitedJSONTransportTest, ReadMultipleMessages) {
124+
std::string json1 = R"json({"str": "one"})json";
125+
std::string json2 = R"json({"str": "two"})json";
126+
std::string message = formatv("Content-Length: {0}\r\nContent-type: "
127+
"text/json\r\n\r\n{1}Content-Length: "
128+
"{2}\r\nContent-type: text/json\r\n\r\n{3}",
129+
json1.size(), json1, json2.size(), json2)
130+
.str();
131+
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
132+
Succeeded());
133+
unsigned count = 0;
134+
RunUntil<JSONTestType>([&](Expected<JSONTestType> message) -> bool {
135+
if (count == 0) {
136+
EXPECT_THAT_EXPECTED(message,
137+
HasValue(testing::FieldsAre(/*str=*/"one")));
138+
} else if (count == 1) {
139+
EXPECT_THAT_EXPECTED(message,
140+
HasValue(testing::FieldsAre(/*str=*/"two")));
141+
}
142+
143+
count++;
144+
return count < 2;
108145
});
109146
}
110147

@@ -115,10 +152,8 @@ TEST_F(HTTPDelimitedJSONTransportTest, ReadAcrossMultipleChunks) {
115152
formatv("Content-Length: {0}\r\n\r\n{1}", json.size(), json).str();
116153
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
117154
Succeeded());
118-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
119-
ASSERT_THAT_EXPECTED(message,
120-
HasValue(testing::FieldsAre(/*str=*/long_str)));
121-
});
155+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
156+
HasValue(testing::FieldsAre(/*str=*/long_str)));
122157
}
123158

124159
TEST_F(HTTPDelimitedJSONTransportTest, ReadPartialMessage) {
@@ -134,16 +169,13 @@ TEST_F(HTTPDelimitedJSONTransportTest, ReadPartialMessage) {
134169
ASSERT_THAT_EXPECTED(input.Write(part2.data(), part2.size()), Succeeded());
135170
});
136171

137-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
138-
ASSERT_THAT_EXPECTED(message, HasValue(testing::FieldsAre(/*str=*/"foo")));
139-
});
172+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
173+
HasValue(testing::FieldsAre(/*str=*/"foo")));
140174
}
141175

142176
TEST_F(HTTPDelimitedJSONTransportTest, ReadWithEOF) {
143177
input.CloseWriteFileDescriptor();
144-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
145-
ASSERT_THAT_EXPECTED(message, Failed<TransportEOFError>());
146-
});
178+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(), Failed<TransportEOFError>());
147179
}
148180

149181
TEST_F(HTTPDelimitedJSONTransportTest, ReaderWithUnhandledData) {
@@ -156,9 +188,8 @@ TEST_F(HTTPDelimitedJSONTransportTest, ReaderWithUnhandledData) {
156188
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size() - 1),
157189
Succeeded());
158190
input.CloseWriteFileDescriptor();
159-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
160-
ASSERT_THAT_EXPECTED(message, Failed<TransportUnhandledContentsError>());
161-
});
191+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
192+
Failed<TransportUnhandledContentsError>());
162193
}
163194

164195
TEST_F(HTTPDelimitedJSONTransportTest, InvalidTransport) {
@@ -184,18 +215,16 @@ TEST_F(JSONRPCTransportTest, MalformedRequests) {
184215
ASSERT_THAT_EXPECTED(
185216
input.Write(malformed_header.data(), malformed_header.size()),
186217
Succeeded());
187-
RunOnce<JSONTestType>(
188-
[&](auto message) { ASSERT_THAT_EXPECTED(message, llvm::Failed()); });
218+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(), llvm::Failed());
189219
}
190220

191221
TEST_F(JSONRPCTransportTest, Read) {
192222
std::string json = R"json({"str": "foo"})json";
193223
std::string message = formatv("{0}\n", json).str();
194224
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
195225
Succeeded());
196-
RunOnce<JSONTestType>([&](auto message) {
197-
ASSERT_THAT_EXPECTED(message, HasValue(testing::FieldsAre(/*str=*/"foo")));
198-
});
226+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
227+
HasValue(testing::FieldsAre(/*str=*/"foo")));
199228
}
200229

201230
TEST_F(JSONRPCTransportTest, ReadAcrossMultipleChunks) {
@@ -204,10 +233,8 @@ TEST_F(JSONRPCTransportTest, ReadAcrossMultipleChunks) {
204233
std::string message = formatv("{0}\n", json).str();
205234
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size()),
206235
Succeeded());
207-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
208-
ASSERT_THAT_EXPECTED(message,
209-
HasValue(testing::FieldsAre(/*str=*/long_str)));
210-
});
236+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
237+
HasValue(testing::FieldsAre(/*str=*/long_str)));
211238
}
212239

213240
TEST_F(JSONRPCTransportTest, ReadPartialMessage) {
@@ -222,16 +249,13 @@ TEST_F(JSONRPCTransportTest, ReadPartialMessage) {
222249
ASSERT_THAT_EXPECTED(input.Write(part2.data(), part2.size()), Succeeded());
223250
});
224251

225-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
226-
ASSERT_THAT_EXPECTED(message, HasValue(testing::FieldsAre(/*str=*/"foo")));
227-
});
252+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
253+
HasValue(testing::FieldsAre(/*str=*/"foo")));
228254
}
229255

230256
TEST_F(JSONRPCTransportTest, ReadWithEOF) {
231257
input.CloseWriteFileDescriptor();
232-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
233-
ASSERT_THAT_EXPECTED(message, Failed<TransportEOFError>());
234-
});
258+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(), Failed<TransportEOFError>());
235259
}
236260

237261
TEST_F(JSONRPCTransportTest, ReaderWithUnhandledData) {
@@ -241,9 +265,8 @@ TEST_F(JSONRPCTransportTest, ReaderWithUnhandledData) {
241265
ASSERT_THAT_EXPECTED(input.Write(message.data(), message.size() - 1),
242266
Succeeded());
243267
input.CloseWriteFileDescriptor();
244-
RunOnce<JSONTestType>([&](llvm::Expected<JSONTestType> message) {
245-
ASSERT_THAT_EXPECTED(message, Failed<TransportUnhandledContentsError>());
246-
});
268+
ASSERT_THAT_EXPECTED(RunOnce<JSONTestType>(),
269+
Failed<TransportUnhandledContentsError>());
247270
}
248271

249272
TEST_F(JSONRPCTransportTest, Write) {

0 commit comments

Comments
 (0)