Repository navigation
Preserve the trailer boundary when deleting or extracting header fields. - #125
Open
samuel-williams-shopify wants to merge 5 commits into
Open
samuel-williams-shopify wants to merge 5 commits into
samuel-williams-shopify wants to merge 5 commits into
Conversation
This was referenced Nov 4, 2026
… remove the matching fields.
…nt, a key straddling the boundary, and no trailers.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Protocol::HTTP::Headers#deleteand#extractnow adjust the trailer boundary (@tail) when they remove fields from the header section, so trailers added aftertrailer!stay in the trailer section rather than leaking into the header section.#deletealso 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.deleteandextractrebuilt@fieldsviapartitionwithout touching@tail, so removing a field from the header section (index below@tail) shifted the first trailer into the header range. For example, with headersa,b, thentrailer!, thenadd("etag", ...), callingdelete("a")lefttrailerempty andheaderas[["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[]=andset, which go throughdelete.Separately,
deletedowncased the key for the@indexedearly-exit check but comparedfield.first.downcase == keyusing the raw key, sodelete("Content-Type")never matched:set("Content-Type", ...)left both the old and new fields in place, and when indexed,delete("Content-Type")returnednileven though the field existed.Solution
Both
deleteandextractnow share a privateremove_fieldshelper. When there is no trailer boundary (@tailisnil, which is the case for every request and for responses at the pointprotocol-http1deletestransfer-encoding/content-length), it uses the originalpartitionpath unchanged. Only when a boundary exists does it walk@fieldswith an index, collecting the removed and kept fields and decrementing@tailfor every removed field whose index is below the boundary. Removing a field from the trailer section leaves@tailunchanged, and the early-exit path indelete(when the index shows the key is absent) is unaffected.@indexedneeds no changes since it is keyed by name, not position. The other mutators were already consistent:clearresets@tail,initialize_dupcopies it, andadd/merge!only append.deletenow doeskey = key.downcaseup front so the index check and the field predicate agree.extractalready required lowercase keys consistently (all callers in socketry repos pass lowercase constants or parsedConnectionvalues, which are lowercased), so its doc comment now states that rather than adding a per-callmap(&:downcase).Performance
The first iteration of
remove_fieldsalways usedeach_with_indexwith a nestedyield, which regressed the common no-trailer path.benchmark-ips, 10 fields and 2 deletes (transfer-encoding,content-length), Ruby 4.0.9 arm64:main(partition)The remaining difference is the extra method call and block forwarding.
Notes for reviewers
Headers#[]=. #126 (validate-trailer-assignment): both insert a method afterdeleteinheaders.rb, both add tests after the#setblock, and both add a## Unreleasedsection toreleases.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 usesserver-timing(which is permitted as a trailer) rather thancontent-type, so it continues to pass once Validate trailers when assigning headers withHeaders#[]=. #126 makes[]=validate the trailer policy.attr :fieldsandto_ahand out the live@fieldsarray, 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 throughHeadersitself.Tests cover
delete(single field, multi-field, indexed, missing, mixed case key with and without an index),setwith a mixed case key,extract(partial and all header fields), deleting/extracting trailer-section fields, a key present in both sections (tailmoves by exactly one),[]=replacing a trailer-permitted header-section field aftertrailer!, andtrailer!with no trailers followed bydelete(tailstays atfields.size), checkingtail,trailer?,header, andtrailerin each case.