Copilot commented on code in PR #3629:
URL: https://github.com/apache/brpc/pull/3629#discussion_r4322784037
##########
src/brpc/details/http_message.cpp:
##########
@@ -550,7 +565,7 @@ ssize_t HttpMessage::ParseFromIOBuf(const butil::IOBuf
&buf) {
&_parser, &g_parser_settings, blk.data(), blk.size());
nprocessed += n;
_parsed_block_size += n;
- if (_parser.http_errno != 0) {
+ if (!ParserOk(_parser)) {
// May try HTTP on other formats, failure is norm.
RPC_VLOG << "Fail to parse http message, parser=" << _parser
<< ", buf=" << butil::ToPrintable(buf);
Review Comment:
When the parser is paused at `on_message_complete`, `http_parser_execute`
will stop mid-block, but `ParseFromIOBuf` continues iterating subsequent blocks
and calling `http_parser_execute` while still paused. This adds avoidable work
for multi-block buffers; consider breaking out of the block-iteration loop once
the message is completed/paused (e.g., detect `HPE_PAUSED` or `Completed()`
after each execute) so the function returns immediately after consuming exactly
one message.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -3081,6 +3082,61 @@ TEST_F(HttpTest, http_expect) {
ASSERT_EQ(imsg_guard->header().status_code(), brpc::HTTP_STATUS_OK);
}
+TEST_F(HttpTest, pipelined_requests_get_one_response_each) {
+ brpc::Server server;
+ server.set_version("pipelined");
+ ASSERT_EQ(0, server.Start(0, nullptr));
+
+ const butil::EndPoint ep = server.listen_address();
+ brpc::SocketOptions options;
+ options.remote_side = ep;
+ brpc::SocketId id;
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ brpc::SocketUniquePtr sock;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ // Both requests in one write, so that the server reads them together.
+ butil::IOBuf request_buf;
+ request_buf.append("GET /health HTTP/1.1\r\nHost: a.com\r\n\r\n"
+ "GET /version HTTP/1.1\r\nHost: a.com\r\n\r\n");
+ ASSERT_EQ(0, sock->Write(&request_buf));
+ int64_t start_time = butil::cpuwide_time_us();
+ while (sock->fd() < 0) {
+ bthread_usleep(1000);
+ ASSERT_LT(butil::cpuwide_time_us(), start_time + 1000000L) << "Too
long!";
+ }
+
+ // The server may answer in any order.
+ std::multiset<std::string> bodies;
+ butil::IOPortal read_buf;
+ start_time = butil::cpuwide_time_us();
+ while (bodies.size() < 2) {
+ const ssize_t nr = read_buf.append_from_file_descriptor(sock->fd(),
4096);
+ if (nr < 0 && errno == EAGAIN) {
+ ASSERT_LT(butil::cpuwide_time_us(), start_time + 1000000L)
+ << "Too long! bodies.size()=" << bodies.size();
+ bthread_usleep(1000);
+ continue;
+ }
+ ASSERT_GT(nr, 0);
Review Comment:
This loop only retries on `EAGAIN`. A syscall can also fail with `EINTR`,
which would currently hard-fail the test via `ASSERT_GT(nr, 0)`. Consider
retrying on `EINTR` as well (similar to `EAGAIN`) to reduce test flakiness on
signal-interrupted reads.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -605,6 +611,141 @@ TEST(HttpMessageTest, chunked_trailer_is_not_a_header) {
ASSERT_EQ("10.0.0.1", *xff);
}
+// Puts `data' in a single IOBuf block, the way one read() returns pipelined
+// messages that arrive together.
+void AppendInOneBlock(butil::IOBuf* buf, const std::string& data) {
+ char* copy = (char*)malloc(data.size());
+ memcpy(copy, data.data(), data.size());
+ ASSERT_EQ(0, buf->append_user_data(copy, data.size(), free));
+}
Review Comment:
This helper manually manages heap memory and assumes allocation succeeds. To
make the test helper more robust and self-contained, consider guarding against
allocation failure (and/or using a safer allocation pattern consistent with the
codebase) before calling `memcpy` and `append_user_data`.
##########
src/brpc/details/http_message.h:
##########
@@ -55,11 +55,14 @@ class HttpMessage {
butil::IOBuf &body() { return _body; }
// Parse from array, length=0 is treated as EOF.
+ // Parsing stops at the end of the message, bytes after it are left to
+ // the next message.
// Returns bytes parsed, -1 on failure.
ssize_t ParseFromArray(const char *data, const size_t length);
-
+
// Parse from butil::IOBuf.
// Emtpy `buf' is sliently ignored, which is different from ParseFromArray.
+ // Parsing stops at the end of the message as in ParseFromArray.
// Returns bytes parsed, -1 on failure.
Review Comment:
Fix spelling in the comment: 'Emtpy' -> 'Empty' and 'sliently' -> 'silently'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
