Copilot commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4270433018
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
Review Comment:
This test intends to exercise `HeaderMap` growth/rehash, but inserting 128
headers may not reliably trigger a rehash depending on the map's initial bucket
count and growth policy, making the regression coverage flaky/ineffective.
Consider asserting that a rehash actually occurred (e.g., by tracking bucket
count before/after if accessible in the test) or looping until bucket count
changes with a reasonable upper bound.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+
+ brpc::HttpHeader other_header;
+ other_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ header.Swap(other_header);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
+ value = other_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ header.Clear();
+ ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
Review Comment:
Since the regression includes `Clear()` leaving `_first_set_cookie` stale,
this test should assert the internal cached pointer is reset (e.g.,
`ASSERT_EQ(header._first_set_cookie, nullptr);`) in addition to verifying
`GetHeader(SET_COOKIE)` is false. Otherwise the specific bug fix in `Clear()`
isn't directly validated.
##########
src/brpc/http_header.cpp:
##########
@@ -136,22 +139,34 @@ std::string& HttpHeader::GetOrAddHeader(const
std::string& key) {
std::string* val = _headers.seek(key);
if (nullptr == val) {
+ const size_t old_bucket_count = _headers.bucket_count();
val = _headers.insert({ key, "" });
if (is_set_cookie) {
_first_set_cookie = val;
}
+ if (_headers.bucket_count() != old_bucket_count) {
+ RefreshFirstSetCookie();
+ }
Review Comment:
The `old_bucket_count` + `bucket_count()` comparison + refresh pattern is
duplicated (also in `AddHeader`). Consider extracting a small helper (e.g.,
`MaybeRefreshFirstSetCookieOnRehash(old_bucket_count)`) to keep the
refresh-on-rehash behavior consistent and easier to update.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -883,6 +883,23 @@ TEST(HttpMessageTest, http_header) {
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
ASSERT_EQ(header._first_set_cookie, nullptr);
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ for (int i = 0; i < 128; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
Review Comment:
Indentation inside the loop is inconsistent with surrounding lines in this
block. Aligning indentation to the file's prevailing style improves readability
and avoids style-only diffs later.
--
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]
