Copilot commented on code in PR #3430:
URL: https://github.com/apache/brpc/pull/3430#discussion_r3746226144
##########
test/brpc_grpc_protocol_unittest.cpp:
##########
@@ -272,4 +283,43 @@ TEST_F(GrpcTest, GrpcTimeOut) {
}
}
+TEST(GrpcProtocol, client_sends_large_request_with_small_remote_window) {
+ WindowGrpcService service;
+ brpc::Server server;
+ ASSERT_EQ(0, server.AddService(&service, brpc::SERVER_DOESNT_OWN_SERVICE));
+ brpc::ServerOptions server_options;
+ server_options.h2_settings.stream_window_size = 32;
+ ASSERT_EQ(0, server.Start("127.0.0.1:8012", &server_options));
+
Review Comment:
Binding this test server to a fixed port can cause flakes when tests run
concurrently or when the port is already in use. Prefer letting the OS choose
an ephemeral port (":0").
This issue also appears on line 294 of the same file.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1554,15 +1556,15 @@ TEST_F(HttpTest, http2_window_used_up) {
NULL, &cntl, request_buf, NULL);
butil::IOBuf dummy;
butil::Status st = socket_message->AppendAndDestroySelf(&dummy,
_h2_client_sock.get());
+ ASSERT_TRUE(st.ok());
if (i == nsuc) {
- // the last message should fail according to flow control policy.
- ASSERT_FALSE(st.ok());
- ASSERT_TRUE(st.error_code() == brpc::ELIMIT);
-
ASSERT_TRUE(butil::StringPiece(st.error_str()).starts_with("remote_window_left
is not enough"));
+ ASSERT_GT(ctx->_pending_data_size, 0u);
+ h2_req->DestroyStreamUserData(
+ _h2_client_sock, &cntl, ECANCELED, false);
+ ASSERT_EQ(0u, ctx->_pending_data_size);
Review Comment:
This test reads `ctx->_pending_data_size`, which is a private member of
`H2Context` and will not compile. Prefer asserting via the existing
`Describable` output (which now includes `pending_data_size`) instead of
accessing internals directly.
##########
test/brpc_h2_unsent_message_unittest.cpp:
##########
@@ -27,11 +27,211 @@
#include "brpc/policy/http2_rpc_protocol.h"
#include "gperftools_helper.h"
Review Comment:
This unit test directly accesses H2Context/H2StreamContext private members
(e.g. `_remote_settings`, `_remote_window_left`, `_pending_data_size`,
`_stream_mutex`). As written, it won’t compile unless those members are made
accessible in unit tests.
This issue also appears on line 56 of the same file.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1538,6 +1645,10 @@ H2UnsentRequest::AppendAndDestroySelf(butil::IOBuf* out,
Socket* socket) {
if (ctx->VolatilePendingStreamSize() >
ctx->remote_settings().max_concurrent_streams) {
return butil::Status(ELIMIT, "Pending Stream count exceeds max
concurrent stream");
}
+ if (ctx->PendingDataOvercrowded()) {
+ return butil::Status(EOVERCROWDED,
+ "Too much pending HTTP/2 request data");
+ }
Review Comment:
The pending-request-data limit check (`PendingDataOvercrowded()`) happens
before this request buffers any DATA, and it isn’t coordinated with the
subsequent buffering under `_stream_mutex`. With concurrent requests, multiple
writers can pass the check and then all add pending bytes, exceeding the
configured limit (and also potentially buffering even when the connection is
already overcrowded by the time `AppendClientRequestData()` runs). Consider
enforcing the limit atomically with the decision to buffer (e.g., reserve
pending bytes under `_stream_mutex` based on `min(conn_window, stream_window)`
so only the unsent portion counts, or make `AppendClientRequestData` return a
Status and fail before mutating state).
--
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]
