You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "wwbmmm (via GitHub)" <gi...@apache.org> on 2026/10/20 07:48:20 UTC
[PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
wwbmmm opened a new pull request, #3630:
URL: https://github.com/apache/brpc/pull/3630
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
`H2UnsentRequest::AppendAndDestroySelf()` HPACK-encoded the request headers with the connection's encoder before calling `H2Context::TryToInsertClientStream()`. Encoding adds header fields to the encoder's dynamic table, so when the insert failed (ELIMIT once the open streams reach the server's `SETTINGS_MAX_CONCURRENT_STREAMS`, EOVERCROWDED when unsent DATA exceeds `-socket_max_unwritten_bytes`, or ELOGOFF after a GOAWAY), `Socket::WriteRequest::Setup` dropped the appended HEADERS frame while the encoder's dynamic table kept the new entries. The peer's decoder then lags the encoder, so later requests on the same connection are decoded with header fields of other requests (e.g. another `:path` or another `authorization` value), or fail with a compression error. ELIMIT and EOVERCROWDED are retried by the default RetryPolicy, and h2 only uses CONNECTION_TYPE_SINGLE, so a retried request itself can be decoded with the wrong fields.
### What is changed and the side effects?
Changed:
- Insert the stream before encoding the headers (as before 9aa41f79), so a refused request no longer mutates HPACK state. The DATA frames allowed by the current flow-control windows are collected separately and appended after the HEADERS, so frames reach the wire in the same order as before.
- Added `H2UnsentMessage.append_and_destroy_self_keeps_hpack_state_on_refused_insert`, which decodes what the client writes with a separate `HPacker` standing for the server, and made `request_does_not_fail_when_body_exceeds_window` check that HEADERS are written before DATA. Without the source change the new test fails on every run.
Side effects:
- Performance effects: none, the emitted frames and their order are unchanged.
- Breaking backward compatibility: none.
Tests run:
- `brpc_h2_unsent_message_unittest` (all 6 cases pass)
- `brpc_grpc_protocol_unittest` (all 8 cases pass)
- `brpc_http_rpc_protocol_unittest` (61/62 pass; `HttpTest.parse_http_address` fails in the local sandbox because it resolves `www.baidu.com`, unrelated to this change)
---
🤖 This PR was automatically created by brpc-oncall
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
Posted by "mardgez (via GitHub) via dev" <de...@brpc.apache.org>.
mardgez commented on PR #3630:
URL: https://github.com/apache/brpc/pull/3630#issuecomment-6640883685
Hi @wwbmmm @chenBright, a gentle ping on this one: #3629 and #3631 have landed, and this PR is green and mergeable. Is anything still needed from my side before it can be merged? Thanks!
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3630:
URL: https://github.com/apache/brpc/pull/3630#issuecomment-6306543423
[replied by brpc-oncall robot] The `compile-with-bazel` job failed before compiling anything: the runner could not download the Bazel binary (`dial tcp: lookup releases.bazel.build: i/o timeout`), so this is an infrastructure/network failure unrelated to this change. The corresponding CMake build and the affected unit tests pass locally.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Keep h2 client HPACK state in sync when a stream is refused (brpc)
Posted by "chenBright (via GitHub) via dev" <de...@brpc.apache.org>.
chenBright merged PR #3630:
URL: https://github.com/apache/brpc/pull/3630
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org