Skip to content

Preserve the trailer boundary when deleting or extracting header fields. - #125

Open
samuel-williams-shopify wants to merge 5 commits into
mainfrom
preserve-trailer-boundary
Open

samuel-williams-shopify wants to merge 5 commits into
mainfrom
preserve-trailer-boundary

Conversation

@samuel-williams-shopify

@samuel-williams-shopify samuel-williams-shopify commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Protocol::HTTP::Headers#delete and #extract now adjust the trailer boundary (@tail) when they remove fields from the header section, so trailers added after trailer! stay in the trailer section rather than leaking into the header section. #delete also normalizes its key to lowercase, fixing a pre-existing bug where mixed case keys were not removed.

Problem

trailer! records the trailer boundary as an integer index into @fields. delete and extract rebuilt @fields via partition without touching @tail, so removing a field from the header section (index below @tail) shifted the first trailer into the header range. For example, with headers a, b, then trailer!, then add("etag", ...), calling delete("a") left trailer empty and header as [["b", "2"], ["etag", "\"x\""]] — the trailer would be sent as a header (or, if the headers were already sent, never emitted at all). The same applied to []= and set, which go through delete.

Separately, delete downcased the key for the @indexed early-exit check but compared field.first.downcase == key using the raw key, so delete("Content-Type") never matched: set("Content-Type", ...) left both the old and new fields in place, and when indexed, delete("Content-Type") returned nil even though the field existed.

Solution

Both delete and extract now share a private remove_fields helper. When there is no trailer boundary (@tail is nil, which is the case for every request and for responses at the point protocol-http1 deletes transfer-encoding/content-length), it uses the original partition path unchanged. Only when a boundary exists does it walk @fields with an index, collecting the removed and kept fields and decrementing @tail for every removed field whose index is below the boundary. Removing a field from the trailer section leaves @tail unchanged, and the early-exit path in delete (when the index shows the key is absent) is unaffected. @indexed needs no changes since it is keyed by name, not position. The other mutators were already consistent: clear resets @tail, initialize_dup copies it, and add/merge! only append.

delete now does key = key.downcase up front so the index check and the field predicate agree. extract already required lowercase keys consistently (all callers in socketry repos pass lowercase constants or parsed Connection values, which are lowercased), so its doc comment now states that rather than adding a per-call map(&:downcase).

Performance

The first iteration of remove_fields always used each_with_index with a nested yield, which regressed the common no-trailer path. benchmark-ips, 10 fields and 2 deletes (transfer-encoding, content-length), Ruby 4.0.9 arm64:

Implementation i/s
main (partition) 424k
PR, before the fast path 357k (1.18× slower)
PR, with the fast path 407k (1.04× slower)

The remaining difference is the extra method call and block forwarding.

Notes for reviewers

  • This overlaps textually with Validate trailers when assigning headers with Headers#[]=. #126 (validate-trailer-assignment): both insert a method after delete in headers.rb, both add tests after the #set block, and both add a ## Unreleased section to releases.md. The resolution in every case is "keep both"; a scratch merge of the two branches with that resolution passes the full suite. The []=-after-trailer! test here deliberately uses server-timing (which is permitted as a trailer) rather than content-type, so it continues to pass once Validate trailers when assigning headers with Headers#[]=. #126 makes []= validate the trailer policy.
  • attr :fields and to_a hand out the live @fields array, so a caller mutating it directly could still desync @tail. No such callers exist in the socketry repos; this PR only guarantees consistency for mutations made through Headers itself.

Tests cover delete (single field, multi-field, indexed, missing, mixed case key with and without an index), set with a mixed case key, extract (partial and all header fields), deleting/extracting trailer-section fields, a key present in both sections (tail moves by exactly one), []= replacing a trailer-permitted header-section field after trailer!, and trailer! with no trailers followed by delete (tail stays at fields.size), checking tail, trailer?, header, and trailer in each case.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant