wwbmmm commented on code in PR #3630:
URL: https://github.com/apache/brpc/pull/3630#discussion_r4332425954
##########
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:
[replied by brpc-oncall robot] The frames decoded here are produced by
brpc's own `PackH2Headers()`, which only sets `H2_FLAGS_END_STREAM` /
`H2_FLAGS_END_HEADERS` and never `PADDED` or `PRIORITY`, so there is no padding
or priority prefix to strip. Draining the whole buffer is intentional: one
header block may span a HEADERS frame plus CONTINUATION frames, and the same
buffer also carries DATA frames that `DecodeHeaderBlock` must skip.
##########
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:
[replied by brpc-oncall robot] The stream IDs are deterministic in this
setup: `AllocateClientStreamId()` continues from `_last_sent_stream_id = 1` in
steps of 2, so the three requests are 1, 3 and 5. Keeping them explicit makes
the `max_concurrent_streams = 1` refusal scenario easy to follow, and the other
tests in this file (e.g. `clear_pending_data_releases_overcrowded_buffer`) also
use fixed IDs 1 and 3.
##########
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:
[replied by brpc-oncall robot] `H2_FLAGS_END_STREAM` is a file-static
constant defined in `http2_rpc_protocol.cpp`, not in the header, so the test
cannot use it, and the other assertions in this file already use `frame.flags &
0x1`. Moving the constant to the public header is out of scope for this fix.
##########
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:
[replied by brpc-oncall robot] `ctx` is not leaked: after
`sock->initialize_parsing_context(&ctx)` the `H2Context` (a `Destroyable`) is
owned by the socket's parsing context and released by the socket lifecycle,
exactly like the other tests in this file. The test must not `delete` it, since
the socket still holds the pointer.
--
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]
