Copilot commented on code in PR #3630:
URL: https://github.com/apache/brpc/pull/3630#discussion_r4322798020
##########
test/brpc_h2_unsent_message_unittest.cpp:
##########
@@ -244,6 +297,57 @@ TEST(H2UnsentMessage,
clear_pending_data_releases_overcrowded_buffer) {
EXPECT_EQ(0u, ctx->_pending_data_size);
}
+TEST(H2UnsentMessage,
append_and_destroy_self_keeps_hpack_state_on_refused_insert) {
+ brpc::SocketId id;
+ brpc::SocketUniquePtr sock;
+ brpc::SocketOptions options;
+ options.user = brpc::get_client_side_messenger();
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(sock.get(), nullptr);
+ ASSERT_EQ(0, ctx->Init());
+ sock->initialize_parsing_context(&ctx);
+ ctx->_last_sent_stream_id = 1;
+ ctx->_remote_settings.max_concurrent_streams = 1;
+
+ // The HPACK decoder of the server, which sees only the HEADERS that
+ // were written to the connection.
+ brpc::HPacker peer;
+ ASSERT_EQ(0, peer.Init(ctx->_unack_local_settings.header_table_size));
+ std::map<std::string, std::string> headers;
+
+ butil::IOBuf out;
+ ASSERT_TRUE(AppendRequest(sock.get(), "http://example.com/status";, "",
+ &out).ok());
+ ASSERT_TRUE(DecodeHeaderBlock(&peer, &out, &headers));
+ delete ctx->RemoveStreamAndDeferWU(1);
+
+ ASSERT_TRUE(AppendRequest(sock.get(), "http://example.com/user";,
+ "Bearer alice", &out).ok());
+ ASSERT_TRUE(DecodeHeaderBlock(&peer, &out, &headers));
+ EXPECT_EQ("/user", headers[":path"]);
+ EXPECT_EQ("Bearer alice", headers["authorization"]);
+
+ // Stream 3 takes the only stream the server allows, so this request is
+ // refused and Socket::WriteRequest::Setup drops what it appended.
+ const butil::Status refused = AppendRequest(
+ sock.get(), "http://example.com/user";, "Bearer bob", &out);
+ EXPECT_EQ(brpc::ELIMIT, refused.error_code());
+ EXPECT_EQ(nullptr, ctx->FindStream(5));
Review Comment:
This test hard-codes stream IDs (1/3/5), which makes it brittle to internal
stream-id allocation changes. A more robust approach is to read the `stream_id`
from the emitted HEADERS frame(s) (via `PopFrame`) and use those IDs for
`RemoveStreamAndDeferWU`/`FindStream` assertions.
##########
test/brpc_h2_unsent_message_unittest.cpp:
##########
@@ -61,6 +63,44 @@ brpc::policy::H2FrameHead PopFrame(butil::IOBuf* buf,
std::string* payload) {
return frame;
}
+// Decode the header block carried by the HEADERS and CONTINUATION frames in
+// |buf| with |peer|, the HPACK decoder of the remote side.
+// Returns false if the block cannot be decoded.
+bool DecodeHeaderBlock(brpc::HPacker* peer, butil::IOBuf* buf,
+ std::map<std::string, std::string>* headers) {
+ butil::IOBuf block;
+ std::string payload;
+ while (!buf->empty()) {
+ const brpc::policy::H2FrameHead frame = PopFrame(buf, &payload);
+ if (frame.type == brpc::policy::H2_FRAME_HEADERS ||
+ frame.type == brpc::policy::H2_FRAME_CONTINUATION) {
+ block.append(payload);
+ }
+ }
Review Comment:
`DecodeHeaderBlock` appends the entire HEADERS payload as if it were a pure
HPACK block fragment. In HTTP/2, HEADERS can include optional PADDED and
PRIORITY fields (depending on flags), which are not part of the HPACK header
block and would cause decoding to fail or produce incorrect results if those
flags are ever set. Consider parsing HEADERS flags to strip padding/priority
fields before appending, and optionally stop after END_HEADERS rather than
draining the entire buffer.
##########
test/brpc_h2_unsent_message_unittest.cpp:
##########
@@ -244,6 +297,57 @@ TEST(H2UnsentMessage,
clear_pending_data_releases_overcrowded_buffer) {
EXPECT_EQ(0u, ctx->_pending_data_size);
}
+TEST(H2UnsentMessage,
append_and_destroy_self_keeps_hpack_state_on_refused_insert) {
+ brpc::SocketId id;
+ brpc::SocketUniquePtr sock;
+ brpc::SocketOptions options;
+ options.user = brpc::get_client_side_messenger();
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ brpc::policy::H2Context* ctx =
+ new brpc::policy::H2Context(sock.get(), nullptr);
+ ASSERT_EQ(0, ctx->Init());
+ sock->initialize_parsing_context(&ctx);
+ ctx->_last_sent_stream_id = 1;
Review Comment:
`ctx` is heap-allocated in the test and never deleted, which introduces a
persistent leak across the test run. Prefer RAII
(`std::unique_ptr<brpc::policy::H2Context>`) or ensure `delete ctx;` is called
before test exit (and consider resetting the socket parsing context if needed).
##########
test/brpc_h2_unsent_message_unittest.cpp:
##########
@@ -160,6 +200,19 @@ TEST(H2UnsentMessage,
request_does_not_fail_when_body_exceeds_window) {
const butil::Status status =
request->AppendAndDestroySelf(&out, sock.get());
EXPECT_TRUE(status.ok()) << status;
+ // HEADERS (split by max_frame_size) come before the DATA the window
+ // allows, and neither ends the stream.
+ std::string payload;
+ brpc::policy::H2FrameHead frame = PopFrame(&out, &payload);
+ EXPECT_EQ(brpc::policy::H2_FRAME_HEADERS, frame.type);
+ EXPECT_EQ(0, frame.flags & 0x1);
+ while (frame.type != brpc::policy::H2_FRAME_DATA && !out.empty()) {
+ frame = PopFrame(&out, &payload);
+ }
+ EXPECT_EQ(brpc::policy::H2_FRAME_DATA, frame.type);
+ EXPECT_EQ(0, frame.flags & 0x1);
Review Comment:
The `0x1` flag mask is a magic number for END_STREAM. Using the existing
named constant (e.g., `brpc::policy::H2_FLAGS_END_STREAM`) makes the intent
clearer and avoids coupling to the numeric value.
--
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]
