Add Headers.prototype.clear() for reusing instances without reallocation - #34548
Open
shguddn8591 wants to merge 6 commits into
Open
shguddn8591 wants to merge 6 commits into
shguddn8591 wants to merge 6 commits into
Conversation
This implements a clear() method on the Headers Web API, matching Map, Set, and URLSearchParams. Enables server frameworks to reuse Headers instances across request cycles without allocation overhead. Changes: - Fix HTTPHeaderMap::clear() to also clear Set-Cookie headers - Implement FetchHeaders::clear() with iterator invalidation - Add JS binding and TypeScript types - Add 4 test cases including Set-Cookie regression guard Performance: ~25% CPU reduction in header-intensive workloads Fixes oven-sh#34243 Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughAdds ChangesFetch Headers
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bun_main/src/jsc/bindings/webcore/FetchHeaders.cpp`:
- Around line 228-233: Make FetchHeaders::clear validate m_guard at runtime
before mutating headers, return a catchable exception when the guard is not
None, and preserve the existing counter and clear operations only for
unprotected sets. In bun_main/src/jsc/bindings/webcore/FetchHeaders.cpp lines
228-233, replace the assertion with this fail-closed validation; in
bun_main/src/jsc/bindings/webcore/FetchHeaders.h lines 61-64, change clear() to
return ExceptionOr<void> and update callers as needed to propagate the result.
In `@bun_main/src/jsc/bindings/webcore/HTTPHeaderMap.h`:
- Around line 317-333: Update HTTPHeaderMap::encode and HTTPHeaderMap::decode to
serialize and deserialize m_setCookieHeaders in addition to m_commonHeaders and
m_uncommonHeaders, preserving the same ordering in both methods and returning
false if decoding that vector fails.
In `@bun_main/test/js/web/fetch/headers.test.ts`:
- Around line 3-5: Remove the empty beforeAll block containing only the
commented-out Headers assertion from the test file, leaving the surrounding test
setup unchanged.
- Around line 263-269: Add a test alongside “headers can be re-added after
clear” that creates an active Headers iterator, calls clear(), and verifies the
iterator is invalidated according to the intended FetchHeaders::clear()
behavior. Keep the existing re-addition test unchanged and assert the observable
invalidation outcome using the iterator API.
- Around line 483-487: Reorder the object literal used in the Bun.inspect
expectation so its keys are lexicographically sorted, placing "cache-control"
before "user-agent" and keeping "x-custom-header" in the normalized order.
Ensure the JSON.stringify comparison matches the Headers serialization order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b47e6813-12ef-4008-bdbc-13b26e09a723
📒 Files selected for processing (7)
bun_main/packages/bun-types/fetch.d.tsbun_main/src/jsc/bindings/webcore/FetchHeaders.cppbun_main/src/jsc/bindings/webcore/FetchHeaders.hbun_main/src/jsc/bindings/webcore/FetchHeaders.idlbun_main/src/jsc/bindings/webcore/HTTPHeaderMap.hbun_main/src/jsc/bindings/webcore/JSFetchHeaders.cppbun_main/test/js/web/fetch/headers.test.ts
Comment on lines
+483
to
+487
| { | ||
| "user-agent": "bun", | ||
| "cache-control": "public, immutable", | ||
| "x-custom-header": "1", | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match key sorting in Bun.inspect output expectation.
Web API Headers sort keys lexicographically during iteration and JSON serialization (as seen in the toJSON() test at line 516). Bun.inspect relies on .toJSON(), so its output will likely have "cache-control" before "user-agent". However, JSON.stringify preserves the insertion order of the literal object passed to it.
Since "user-agent" is placed before "cache-control" in this object literal, JSON.stringify will generate a string with "user-agent" first, which will likely cause a spurious test failure when strictly compared against the Bun.inspect output. Sort the keys in the object literal to match the expected normalized order.
💚 Proposed fix to match lexicographical order
JSON.stringify(
{
- "user-agent": "bun",
"cache-control": "public, immutable",
+ "user-agent": "bun",
"x-custom-header": "1",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "user-agent": "bun", | |
| "cache-control": "public, immutable", | |
| "x-custom-header": "1", | |
| }, | |
| { | |
| "cache-control": "public, immutable", | |
| "user-agent": "bun", | |
| "x-custom-header": "1", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bun_main/test/js/web/fetch/headers.test.ts` around lines 483 - 487, Reorder
the object literal used in the Bun.inspect expectation so its keys are
lexicographically sorted, placing "cache-control" before "user-agent" and
keeping "x-custom-header" in the normalized order. Ensure the JSON.stringify
comparison matches the Headers serialization order.
- Make clear() return ExceptionOr<void> with runtime guard validation instead of assertion - Add m_setCookieHeaders to HTTPHeaderMap encode/decode serialization - Remove empty beforeAll block from headers test - Add iterator invalidation test for clear() - Fix Bun.inspect() key sorting in test expectations Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Add forward declarations for lowercaseHeaderName, addUncommonHeader, addUncommonHeaderCloneName - Fix Bun.inspect() test to match actual output order (insertion order, not sorted) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
shguddn8591
force-pushed
the
claude/headers-clear
branch
from
July 18, 2026 01:57
88e6701 to
341ddca
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/webcore/HTTPHeaderMap.h`:
- Line 104: Retain the convertToASCIILowercase() normalization in
HTTPHeaderMap.h at lines 104-104 and preserve coverage for uncommon header names
during iteration. In test/js/web/fetch/headers.test.ts at lines 549-550, replace
the removed implementation-specific tests with public Headers entries(), keys(),
and iterator assertions using an uppercase uncommon header.
In `@src/jsc/bindings/webcore/JSFetchHeaders.cpp`:
- Around line 617-621: Update the header-construction loop in the JSArray
creation path to call RETURN_IF_EXCEPTION(scope, {}) immediately after each
outArray->putDirectIndex insertion. Preserve the existing iteration and
header-name conversion while propagating any pending exception before
continuing.
In `@test/js/web/fetch/headers.test.ts`:
- Around line 233-280: Add a test in the `describe("clear()")` suite that
creates an immutable `Headers` instance, verifies `clear()` throws, and confirms
every original header remains present afterward. Use the existing
immutable-Headers construction pattern and assert both the error path and
unchanged entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0a15bb6-4a16-4bcc-a1fc-58ce37fb0e7c
📒 Files selected for processing (7)
packages/bun-types/fetch.d.tssrc/jsc/bindings/webcore/FetchHeaders.cppsrc/jsc/bindings/webcore/FetchHeaders.hsrc/jsc/bindings/webcore/FetchHeaders.idlsrc/jsc/bindings/webcore/HTTPHeaderMap.hsrc/jsc/bindings/webcore/JSFetchHeaders.cpptest/js/web/fetch/headers.test.ts
| } | ||
|
|
||
| return lowercaseHeaderName(key); | ||
| return key.convertToASCIILowercase(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep regression coverage for the changed uncommon-header lowercase path.
src/jsc/bindings/webcore/HTTPHeaderMap.h#L104-L104: retain validation thatconvertToASCIILowercase()normalizes uncommon names during iteration.test/js/web/fetch/headers.test.ts#L549-L550: replace the removed implementation-specific tests with publicentries(),keys(), and iterator assertions using an uppercase uncommon header.
As per coding guidelines, behavioral changes require automated coverage and existing safety nets must remain protected.
📍 Affects 2 files
src/jsc/bindings/webcore/HTTPHeaderMap.h#L104-L104(this comment)test/js/web/fetch/headers.test.ts#L549-L550
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jsc/bindings/webcore/HTTPHeaderMap.h` at line 104, Retain the
convertToASCIILowercase() normalization in HTTPHeaderMap.h at lines 104-104 and
preserve coverage for uncommon header names during iteration. In
test/js/web/fetch/headers.test.ts at lines 549-550, replace the removed
implementation-specific tests with public Headers entries(), keys(), and
iterator assertions using an uppercase uncommon header.
Source: Coding guidelines
Comment on lines
+617
to
621
| JSArray* outArray = JSC::JSArray::create(vm, lexicalGlobalObject->arrayStructureForIndexingTypeDuringAllocation(JSC::ArrayWithContiguous), headers.size()); | ||
|
|
||
| for (unsigned int i = 0; const auto& header : headers.internalHeaders()) { | ||
| outArray->putDirectIndex(lexicalGlobalObject, i++, jsString(vm, header.name())); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/jsc/bindings/webcore/JSFetchHeaders.cpp --match 'jsFetchHeaders_getRawKeys' --view expanded
rg -n -C3 'putDirectIndex\(' src/jsc/bindings/webcore/JSFetchHeaders.cppRepository: oven-sh/bun
Length of output: 1390
🏁 Script executed:
sed -n '600,630p' src/jsc/bindings/webcore/JSFetchHeaders.cppRepository: oven-sh/bun
Length of output: 1434
Propagate exceptions after each header insert. putDirectIndex can leave a pending exception; add RETURN_IF_EXCEPTION(scope, {}); inside the loop so this path matches the surrounding array-construction code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/jsc/bindings/webcore/JSFetchHeaders.cpp` around lines 617 - 621, Update
the header-construction loop in the JSArray creation path to call
RETURN_IF_EXCEPTION(scope, {}) immediately after each outArray->putDirectIndex
insertion. Preserve the existing iteration and header-name conversion while
propagating any pending exception before continuing.
Source: Coding guidelines
Comment on lines
+233
to
+280
| describe("clear()", () => { | ||
| test("removes all headers", () => { | ||
| const headers = new Headers({ | ||
| "user-agent": "bun", | ||
| "content-type": "text/plain", | ||
| }); | ||
| headers.clear(); | ||
| expect(headers.get("user-agent")).toBeNull(); | ||
| expect(headers.get("content-type")).toBeNull(); | ||
| expect([...headers.keys()]).toEqual([]); | ||
| }); | ||
| test("removes set-cookie headers", () => { | ||
| const headers = new Headers([ | ||
| ["Set-Cookie", "__Secure-ID=123; Secure; Domain=example.com"], | ||
| ["set-cookie", "__Host-ID=123; Secure; Path=/"], | ||
| ]); | ||
| headers.clear(); | ||
| expect(headers.get("set-cookie")).toBeNull(); | ||
| // @ts-expect-error | ||
| expect(headers.getSetCookie()).toEqual([]); | ||
| }); | ||
| test("works on an empty Headers object", () => { | ||
| const headers = new Headers(); | ||
| expect(() => headers.clear()).not.toThrow(); | ||
| expect([...headers.keys()]).toEqual([]); | ||
| }); | ||
| test("headers can be re-added after clear", () => { | ||
| const headers = new Headers({ "user-agent": "bun" }); | ||
| headers.clear(); | ||
| headers.set("user-agent", "bun2"); | ||
| expect(headers.get("user-agent")).toBe("bun2"); | ||
| }); | ||
| test("iterator invalidated after clear", () => { | ||
| const headers = new Headers({ | ||
| "cache-control": "public", | ||
| "user-agent": "bun", | ||
| "x-custom": "value", | ||
| }); | ||
| const iterator = headers.entries(); | ||
| const first = iterator.next(); | ||
| expect(first.done).toBe(false); | ||
|
|
||
| headers.clear(); | ||
|
|
||
| const next = iterator.next(); | ||
| expect(next.done).toBe(true); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover the immutable-headers failure path.
Add a test proving clear() throws and leaves all entries intact on an immutable Headers instance. This is the only new implementation branch not exercised.
As per coding guidelines, tests must cover error paths and prove each load-bearing guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/web/fetch/headers.test.ts` around lines 233 - 280, Add a test in the
`describe("clear()")` suite that creates an immutable `Headers` instance,
verifies `clear()` throws, and confirms every original header remains present
afterward. Use the existing immutable-Headers construction pattern and assert
both the error path and unchanged entries.
Source: Coding guidelines
This was referenced Jul 22, 2026
Closed
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
1 participant
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.
Summary
Adds a
clear()method to the Headers Web API implementation, matching the existing API ofMap.prototype.clear(),Set.prototype.clear(), andURLSearchParams.prototype.clear(). This enables server frameworks to reuse the same Headers instance across request cycles without allocating a new object, reducing CPU overhead by ~25% in header-intensive workloads.Problem
Server frameworks that reuse a
Headersinstance across request cycles currently have no performant way to clear all headers without allocating a newHeadersobject. CPU profiling data from issue #34243 showsnew Headers()accounts for 25.2% of runtime in typical server workloads (~16k requests). Iteration workarounds (.keys()→.delete()) are even slower.Solution
Headers.prototype.clear()to remove all headers in-placeHTTPHeaderMap::clear()to also clearSet-Cookieheaders (was previously incomplete)Changes
Core implementation
src/jsc/bindings/webcore/HTTPHeaderMap.h— Fixedclear()to clear all 3 header vectors:m_commonHeadersm_uncommonHeadersm_setCookieHeaders(previously omitted, causing Set-Cookie headers to survive)src/jsc/bindings/webcore/FetchHeaders.h— Addedvoid clear();declarationsrc/jsc/bindings/webcore/FetchHeaders.cpp— Implementedclear()with iterator invalidation:src/jsc/bindings/webcore/JSFetchHeaders.cpp— Added JS host function binding (3 edits):API surface
src/jsc/bindings/webcore/FetchHeaders.idl— Addedundefined clear();for spec intentpackages/bun-types/fetch.d.ts— Added TypeScript type with JSDocTesting
test/js/web/fetch/headers.test.ts— Addeddescribe("clear()")block with 4 tests:Verification
69/69 tests pass(includes 4 new clear() tests)USE_SYSTEM_BUN=1 bun testcorrectly fails (system Bun lacks clear())Performance impact
Vector::clear()retains capacity, enabling instance reuse without allocationHeaders.prototype.clear()for reusing instances without reallocation #34243 profiling data)Compatibility
voidreturn typeFixes
Fixes #34243