[songgaoye]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

HttpHeader caches the first Set-Cookie value as a pointer into its
HeaderMap. The pointer becomes stale when the map grows.

Refreshing it with HeaderMap::seek() is not sufficient: HeaderMap is a
multi-map and does not preserve the identity or insertion order of duplicate
Set-Cookie fields. The refreshed pointer may therefore select a different
cookie.

What is changed and the side effects?

Changed:

Store Set-Cookie fields separately in insertion order and keep HeaderMap
synchronized for header iteration and serialization.

Make GetHeader(SET_COOKIE) and GetAllSetCookieHeader() read the stable
ordered storage. This also makes Swap() and Clear() safe without retaining
a stale cached pointer.

Add regression coverage for actual map growth, multiple Set-Cookie fields,
fragmented HTTP parsing, Swap(), copy construction, and assignment.

Side effects:

• Performance effects:

  The change only adds separate storage and synchronization for Set-Cookie
  fields. Ordinary headers keep the existing path.

• Breaking backward compatibility:

  None.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

• This test intends to exercise HeaderMap growth/rehash, but inserting 128 headers may not reliably… · New
• Since the regression includes Clear() leaving _first_set_cookie stale, this test should assert… · New
• The old_bucket_count + bucket_count() comparison + refresh pattern is duplicated (also in… · New
• Indentation inside the loop is inconsistent with surrounding lines in this block. Aligning… · New

What changed in this PR

Fixes a stale cached pointer for the first Set-Cookie header in HttpHeader after HeaderMap mutations (rehash/growth), Swap(), and Clear(), and adds regression tests for these cases.

Changes:

• Refresh _first_set_cookie after HeaderMap growth and after HttpHeader::Swap()
• Clear _first_set_cookie in HttpHeader::Clear()
• Add unit tests covering map growth, Swap(), and Clear()

File	Description
test/​brpc_http_message_unittest.cpp	Adds regression tests to catch stale _first_set_cookie after growth, Swap(), and Clear().
src/​brpc/​http_header.h	Declares RefreshFirstSetCookie() helper to rebuild the cached pointer safely.
src/​brpc/​http_header.cpp	Implements pointer refresh logic on rehash/growth and Swap(), and clears cache on Clear().

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[songgaoye]

The CI failure is unrelated to this PR. It was fixed in #3610.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

• HttpMessage calls HttpHeader::HasSetCookieHeaders() and HttpHeader::SyncSetCookieHeaders().… · New
• AddHeader() now unconditionally appends into _set_cookie_headers and syncs SET_COOKIE,… · New
• This test forces a HeaderMap growth by inserting up to 4096 headers and asserting… · New
• The counting variables are handled inconsistently here: set_cookie_value1_count is declared… · New

4 resolved since last review

• Since the regression includes Clear() leaving _first_set_cookie stale, this test should assert…
• This test intends to exercise HeaderMap growth/rehash, but inserting 128 headers may not reliably…
• Indentation inside the loop is inconsistent with surrounding lines in this block. Aligning…
• The old_bucket_count + bucket_count() comparison + refresh pattern is duplicated (also in…

🧠 Review effort: Lite

[Copilot]

🟡 Changes recommended

Rebuilding all cookie entries after each append can cause quadratic work for multi-cookie responses.

1 open issue

• Avoid quadratic Set-Cookie synchronization during multi-cookie responses · New

4 resolved since last review

• AddHeader() now unconditionally appends into _set_cookie_headers and syncs SET_COOKIE,…
• HttpMessage calls HttpHeader::HasSetCookieHeaders() and HttpHeader::SyncSetCookieHeaders().…
• This test forces a HeaderMap growth by inserting up to 4096 headers and asserting…
• The counting variables are handled inconsistently here: set_cookie_value1_count is declared…

_{Review effort: Lite}

[wwbmmm]

LGTM

[wwbmmm]

[replied by brpc-oncall robot] Agreed that this sequence is unsafe, and thanks for the precise reproducer.

The corruption does not come from the new Set-Cookie storage itself, but from butil::FlatMap's multi-map erase(): when the erased key owns the bucket head and a colliding entry survives, the old implementation destroys the head element and then move-assigns the surviving node into that already-destroyed object. With the default 16 buckets, set-cookie and user-agent both land in bucket 3, so the sequence above hits exactly that path (this was also latently reachable before this PR, but only through RemoveHeader()).

Two follow-ups:

1. This defect is already fixed on master by Fix use-after-destruction when erasing MultiFlatMap bucket head #3617 (the head element is no longer destroyed when a survivor is moved up). This branch's base predates that fix — the code quoted above (flat_map_inl.h:473 destroying the head unconditionally) is the pre-fix version. Note that CI builds the PR's merge ref against current master, so the current CI run cannot expose it. Please rebase/merge master so the new _headers.erase(SET_COOKIE) calls in AddHeader() and SyncSetCookieHeaders() run on the fixed implementation; no workaround is needed inside http_header.cpp.

2. Since SetHeader()/SyncSetCookieHeaders() now trigger _headers.erase(SET_COOKIE) on every cookie update (previously only RemoveHeader() did), this collision path becomes far more common. Please add the sequence above as a regression test in test/brpc_http_message_unittest.cpp (Set-Cookie -> colliding header -> Set-Cookie) so the header code keeps exercising it.

[chenBright]

LGTM