Copilot commented on code in PR #3629:
URL: https://github.com/apache/brpc/pull/3629#discussion_r4332519318
##########
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:
If `append_user_data` fails, `ASSERT_EQ` aborts the test without freeing
`copy`, leaking memory in the failing path. Use an RAII owner (e.g.,
`std::unique_ptr<char, decltype(&free)>`) and release it only after
`append_user_data` succeeds.
##########
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!";
+ }
Review Comment:
The timeout value `1000000L` (and related sleep interval) is duplicated.
Consider extracting a named constant (e.g., `constexpr int64_t kTimeoutUs =
...`) to avoid magic numbers and keep the two loops consistent if the value
needs to change.
##########
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 || errno == EINTR)) {
+ ASSERT_LT(butil::cpuwide_time_us(), start_time + 1000000L)
+ << "Too long! bodies.size()=" << bodies.size();
Review Comment:
The timeout value `1000000L` (and related sleep interval) is duplicated.
Consider extracting a named constant (e.g., `constexpr int64_t kTimeoutUs =
...`) to avoid magic numbers and keep the two loops consistent if the value
needs to change.
##########
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:
`AppendInOneBlock` is defined with external linkage in a test TU. This can
cause ODR/linker errors if another test file defines the same helper name. Make
it `static` or place it in an anonymous namespace to keep it TU-local.
--
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]
