wwbmmm commented on code in PR #3629:
URL: https://github.com/apache/brpc/pull/3629#discussion_r4344920518
##########
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:
[replied by brpc-oncall robot] `AppendInOneBlock` is already defined inside
the anonymous namespace that wraps all tests in this file (`namespace brpc {
namespace { ... } }`), so it already has internal linkage and cannot conflict
with other TUs.
##########
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:
[replied by brpc-oncall robot] The helper intentionally uses
`append_user_data` with a dedicated buffer so that the bytes always form
exactly one IOBuf block, independently of how much space is left in the
thread-local block (a plain `IOBuf::append` may split small data over two
blocks). `append_user_data` takes ownership of the buffer on success; on
failure the test has already failed on OOM and there is nothing meaningful to
recover, so I would rather keep the test helper minimal.
##########
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:
[replied by brpc-oncall robot] The same inline `1000000L` timeout is used by
the other tests in this file (e.g. `http_expect`), so I kept the new test
consistent with the existing style instead of introducing a constant for a
single test.
##########
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:
[replied by brpc-oncall robot] The same inline `1000000L` timeout is used by
the other tests in this file (e.g. `http_expect`), so I kept the new test
consistent with the existing style instead of introducing a constant for a
single test.
--
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]
