refactor(Response) isolate body usage #23313
Conversation
cirospaciari
changed the title
cirospaciari
changed the title
WalkthroughRefactors Request/Response/Body internals to use private fields and new constructors/accessors (e.g., Response.init, Request.init2, getBodyValue, getBodyReadableStream, setUrl). Call sites updated to the accessor API, owned ReadableStream ownership propagated, BakeResponse/JS bindings adjusted, and new cyclic-reference tests added. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/api/server/RequestContext.zig (1)
2225-2251: Header access refactor good; fix Content‑Range decision and avoid double header lookup
- Minor logic bug (pre‑existing): mutating needs_content_range suppresses writing Content‑Range when it’s missing. Don’t overwrite the flag; just set status when both are true.
Apply this minimal fix:
- has_content_range = headers_.fastHas(.ContentRange); - needs_content_range = needs_content_range and has_content_range; - if (needs_content_range) { + has_content_range = headers_.fastHas(.ContentRange); + if (needs_content_range and has_content_range) { status = 206; }
- Optional: avoid two passes over headers. Swap once and reuse for content-type detection and emission.
- const content_type, const needs_content_type, const content_type_needs_free = getContentType( - response.getInitHeaders(), - &this.blob, - this.allocator, - ); + const swapped_headers = response.swapInitHeaders(); + const content_type, const needs_content_type, const content_type_needs_free = + getContentType(swapped_headers, &this.blob, this.allocator); defer if (content_type_needs_free) content_type.deinit(this.allocator); - var has_content_disposition = false; - var has_content_range = false; - if (response.swapInitHeaders()) |headers_| { + var has_content_disposition = false; + var has_content_range = false; + if (swapped_headers) |headers_| { defer headers_.deref(); has_content_disposition = headers_.fastHas(.ContentDisposition); has_content_range = headers_.fastHas(.ContentRange); - needs_content_range = needs_content_range and has_content_range; - if (needs_content_range) { + if (needs_content_range and has_content_range) { status = 206; } this.doWriteStatus(status); this.doWriteHeaders(headers_); } else if (needs_content_range) { status = 206; this.doWriteStatus(status); } else { this.doWriteStatus(status); }This keeps later logic intact:
- status 206 when range is needed (with or without header present)
- later block still writes Content‑Range when needed and header absent.
Also applies to: 2287-2303
src/bun.js/webcore/Response.zig (2)
181-186:tryused in function that doesn’t return an error union.
header()returns?[]const u8but usestryonfastGet. IffastGetdoesn’t return an error union,tryis invalid; if it does, propagate the error in the signature.Apply one of:
-pub fn header(this: *const Response, name: bun.webcore.FetchHeaders.HTTPHeaderName) ?[]const u8 { - return if (try (this.#init.headers orelse return null).fastGet(name)) |str| +pub fn header(this: *const Response, name: bun.webcore.FetchHeaders.HTTPHeaderName) ?[]const u8 { + return if ((this.init.headers orelse return null).fastGet(name)) |str| str.slice() else null; }Or, if
fastGetcan throw:-pub fn header(this: *const Response, name: bun.webcore.FetchHeaders.HTTPHeaderName) ?[]const u8 { +pub fn header(this: *const Response, name: bun.webcore.FetchHeaders.HTTPHeaderName) bun.JSError!?[]const u8 { - return if (try (this.#init.headers orelse return null).fastGet(name)) |str| + return if (try (this.init.headers orelse return null).fastGet(name)) |str| str.slice() else null; }
795-802: Inconsistent field naming in struct literal.Mixes
.#bodywith.init. After removing “#”, ensure both fields are consistent:-inline fn emptyWithStatus(_: *jsc.JSGlobalObject, status: u16) Response { - return bun.new(Response, .{ - .#body = Body{ +inline fn emptyWithStatus(_: *jsc.JSGlobalObject, status: u16) Response { + return bun.new(Response, .{ + .body = Body{ .value = Body.Value{ .Null = {} }, }, - .init = Init{ + .init = Init{ .status_code = status, }, }); }
🧹 Nitpick comments (4)
src/bun.js/api/filesystem_router.zig (1)
284-285: Accessor usage is correct; consider deferring slice cleanupSwitch to getUTF8Url aligns with the new API. If it returns a ZigString.Slice requiring deinit, consider deferring path.deinit() so non-rewritten branches don’t leak.
Example (outside the changed lines):
var path: ZigString.Slice = brk: { ... }; defer path.deinit();src/bun.js/webcore/Blob.zig (1)
1342-1400: Response.getBodyValue() refactor looks correct; small optional tidySemantics match prior response.body.value usage. Locked/Error/S3 paths behave the same; toReadableStream() priming before locked.readable access is correct. Optionally, reduce repetition:
- Cache a local alias for bodyValue.Locked when used multiple times.
src/bun.js/api/server/RequestContext.zig (1)
681-691: After abort, consider marking body as UsedYou clear the strong readable and abort it. Consider also setting bodyValue.* = .{ .Used = {} } to prevent any accidental re-consumption later.
src/bun.js/webcore/fetch.zig (1)
1005-1031: Comment mismatch vs behavior (nit).The “Scenario 2b” comment says “keep loading”, but the code ignores body when
promise.isEmptyOrUndefinedOrNull(). Behavior seems right; update the comment for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/bake/DevServer.zig(1 hunks)src/bun.js/api/filesystem_router.zig(1 hunks)src/bun.js/api/html_rewriter.zig(10 hunks)src/bun.js/api/server.zig(1 hunks)src/bun.js/api/server/FileRoute.zig(1 hunks)src/bun.js/api/server/RequestContext.zig(10 hunks)src/bun.js/api/server/StaticRoute.zig(3 hunks)src/bun.js/node/types.zig(1 hunks)src/bun.js/webcore/BakeResponse.zig(2 hunks)src/bun.js/webcore/Blob.zig(2 hunks)src/bun.js/webcore/Request.zig(1 hunks)src/bun.js/webcore/Response.zig(28 hunks)src/bun.js/webcore/fetch.zig(12 hunks)test/internal/ban-limits.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/api/filesystem_router.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bake/DevServer.zigsrc/bun.js/node/types.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/api/html_rewriter.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/api/filesystem_router.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bake/DevServer.zigsrc/bun.js/node/types.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/api/html_rewriter.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/filesystem_router.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/api/server.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/api/html_rewriter.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/api/filesystem_router.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/node/types.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/api/html_rewriter.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/internal/ban-limits.json
🧠 Learnings (1)
📚 Learning: 2025-09-26T01:10:04.233Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: test/internal/ban-limits.json:25-26
Timestamp: 2025-09-26T01:10:04.233Z
Learning: The test/internal/ban-limits.json file contains accurate counts that are validated by tests - if the counts were incorrect, the associated tests would fail, so the values in this file reflect the actual current state of the codebase.
Applied to files:
test/internal/ban-limits.json
🔇 Additional comments (20)
src/bake/DevServer.zig (1)
3355-3364: Response.init wiring looks solidSwitching to
Response.initkeeps the headers/body wiring intact and matches the new constructor shape, so the rejection path still delivers the blob-based payload correctly.src/bun.js/node/types.zig (1)
101-106: Good move to use getBodyValue()Using
response.getBodyValue()aligns this path with the accessor API and keeps the blob conversion logic unchanged.src/bun.js/api/server.zig (1)
1230-1233: Encapsulation via setUrl looks goodBehavior unchanged; avoids direct field mutation. Please confirm setUrl adopts/refs the cloned bun.String to prevent leaks.
src/bun.js/api/server/FileRoute.zig (1)
62-85: BodyValue/headers accessor migration LGTMUsing getBodyValue()+toBlobIfPossible(), reassigning a dupe back into the Response, and sourcing headers via getInitHeaders() preserves behavior and isolates internals.
src/bun.js/webcore/Request.zig (1)
620-649: Correct switch to Response accessors in constructIntoMethod, headers, url, and body now read via getters with proper cloning/dupeRef. Ownership and error checks look correct.
src/bun.js/webcore/BakeResponse.zig (1)
108-121: Use of Response.init and alias is consistentConstruction via Response.init matches the new API and keeps imports at the bottom style. Please verify the init parameter ordering/signature matches Response.init to avoid subtle init bugs.
Also applies to: 140-142
src/bun.js/api/server/StaticRoute.zig (1)
94-153: Accessor-based body/header handling looks solidUsing getBodyValue() with toBlobIfPossible(), use()/dupe(), and getInitHeaders() preserves previous behavior and encapsulates internals. Header sanitization and ETag logic intact.
src/bun.js/api/server/RequestContext.zig (4)
1219-1225: Locked lifecycle during streaming: LGTMRe-establishing .Locked with a strong readable during pending stream, and transitioning to .Used on resolve/reject, ensures abort hooks can find and cancel the stream and prevents reuse.
Also applies to: 1668-1675, 1722-1729
1459-1511: HEAD path via bodyValue: LGTMUsing response.getBodyValue().toBlobIfPossible() and sizing per blob/string/InternalBlob is correct. S3 async stat fallback is preserved.
1565-1580: Protecting Response for stream/file cases: LGTMProtecting the JS value when body is Locked or when Blob needs async file read avoids GC races across render; mirrored for both direct and Promise flows and onError path.
Also applies to: 1625-1639, 2171-2186
1999-2000: Routing render through getBodyValue(): LGTMDirectly passes Response body value pointer to the renderer; aligns with accessor pattern.
src/bun.js/api/html_rewriter.zig (5)
177-187: Accessor migration (getBodyValue) looks correct.Checking body used state via pointer is consistent with the new API. No issues.
202-210: Response.init adoption is sound.Constructing a temporary Response via Response.init and returning the transformed payload is correct. Defer-finalize of resp and out_response is appropriate.
Also applies to: 218-235
421-436: Init/headers/url cloning via setters LGTM.
- setInit with method/status/text clone
- setInitHeaders with cloneThis(...)
- setUrl with clone
All look correct for ownership and refcounts.Also applies to: 475-491
529-564: Error-to-body propagation via Body.Value pointerUsing sink.response.getBodyValue() in both branches is correct. One question: readable.deinit() is called unconditionally when Locked; confirm deinit() is safe when there’s no held readable (Strong is empty).
600-622: Final resolution via replace-and-resolve patternReplacing bodyValue then resolving prev_value with pointer to the new value is correct. Matches fetch-side flow.
src/bun.js/webcore/fetch.zig (2)
968-980: Response.init and Body.Value flow refactor LGTM.
- Response constructed via Response.init across HTTP/data URL/file/S3 paths.
- Body.Value obtained via response.getBodyValue() consistently.
No issues spotted.Also applies to: 1434-1445, 2358-2367, 2541-2574
401-414: Promise rejection path uses toErrorInstance correctly. Verify semantics.On failure, you call
body.toErrorInstance(err, globalThis)and, if aLocked.promiseexists and isn’t handled, reject it. Looks right; please confirm this matches the previous behavior for buffered bodies.src/bun.js/webcore/Response.zig (2)
13-16: Invalid Zig identifiers (“#body”, “#init”, “#url”, “#redirected”, “#reported_estimated_size”).Zig identifiers cannot start with “#”. All occurrences must be regular field names (private by omission of
pub). This will not compile.Apply this pattern (and repeat for all usages in the file):
-#body: Body, -#init: Init, -#url: bun.String = bun.String.empty, -#redirected: bool = false, -#reported_estimated_size: usize = 0, +body: Body, +init: Init, +url: bun.String = bun.String.empty, +redirected: bool = false, +reported_estimated_size: usize = 0,And update all references:
-return &this.#body.value; +return &this.body.value;-this.#init.status_code +this.init.status_code-this.#url +this.urlAlso applies to: 22-23
⛔ Skipped due to learnings
Learnt from: taylordotfish PR: oven-sh/bun#22227 File: src/allocators/allocation_scope.zig:284-314 Timestamp: 2025-09-02T18:25:27.976Z Learning: In bun's custom Zig implementation, the `#` prefix for private fields is valid syntax and should not be flagged as invalid. The syntax `#fieldname` creates private fields that cannot be accessed from outside the defining struct, and usage like `self.#fieldname` is correct within the same struct. This applies to fields like `#parent`, `#state`, `#allocator`, `#trace`, etc. throughout the codebase.Learnt from: taylordotfish PR: oven-sh/bun#22227 File: src/safety/alloc.zig:93-95 Timestamp: 2025-09-02T17:14:46.924Z Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.Learnt from: cirospaciari PR: oven-sh/bun#22842 File: src/bun.js/webcore/ResumableSink.zig:274-276 Timestamp: 2025-09-25T18:14:27.731Z Learning: In Zig code, private fields are declared and accessed using the `#` prefix. When a field is declared as `#field_name`, it must be accessed as `this.#field_name`, not `this.field_name`. The `#` prefix is part of the private field access syntax and should not be removed.
809-831: Verify BodyMixin compatibility: EnsureResponseMixin = BodyMixin(@This())insrc/bun.js/webcore/Response.ziguses the publicbodyfield or thegetBodyValue()accessor after stripping the#prefix; adjust if it still relies on the private#body.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/api/server/RequestContext.zig (1)
2229-2254: Fix Content-Range decision logic (Partial Content status/header)In renderMetadata, needs_content_range is ANDed with has_content_range, which suppresses 206 and header emission when Content-Range is absent. It should emit when needed and not already present.
- has_content_disposition = headers_.fastHas(.ContentDisposition); - has_content_range = headers_.fastHas(.ContentRange); - needs_content_range = needs_content_range and has_content_range; - if (needs_content_range) { + has_content_disposition = headers_.fastHas(.ContentDisposition); + has_content_range = headers_.fastHas(.ContentRange); + // Only set 206 if we need it and the header isn't already present. + if (needs_content_range and !has_content_range) { status = 206; }The later block correctly guards header emission with
if (needs_content_range and !has_content_range) { ... }, so this change aligns status and header behavior.Also applies to: 2291-2306
src/bun.js/webcore/Body.zig (1)
1742-1748: JSValue leak: protected but never unprotectedreadable.protect() is never unprotected; Strong keeps the stream alive already. Drop protect() to avoid leaking the JS ReadableStream handle.
Apply this diff:
- const readable = try value.toReadableStream(sink.global); - readable.ensureStillAlive(); - readable.protect(); + const readable = try value.toReadableStream(sink.global); + readable.ensureStillAlive(); return try sink.bufferLockedBodyValue(value, null);src/bun.js/webcore/Response.zig (1)
871-880: Fix emptyWithStatus: wrong field, wrong return typeCurrently initializes .init (public) and returns Response by value, but fields are private and bun.new returns *Response.
-inline fn emptyWithStatus(_: *jsc.JSGlobalObject, status: u16) Response { - return bun.new(Response, .{ - .#body = Body{ - .value = Body.Value{ .Null = {} }, - }, - .init = Init{ - .status_code = status, - }, - }); -} +inline fn emptyWithStatus(_: *jsc.JSGlobalObject, status: u16) *Response { + return bun.new(Response, .{ + .#body = Body{ + .value = Body.Value{ .Null = {} }, + }, + .#init = Init{ + .status_code = status, + }, + }); +}Also update callers to return pointer:
pub fn @"404"(globalThis: *jsc.JSGlobalObject) *Response { return emptyWithStatus(globalThis, 404); } pub fn @"200"(globalThis: *jsc.JSGlobalObject) *Response { return emptyWithStatus(globalThis, 200); }
♻️ Duplicate comments (1)
src/bun.js/webcore/fetch.zig (1)
405-414: Use promise status check instead of isHandled()isHandled() doesn’t guarantee pending state. Prefer status(vm) != .pending to avoid double-reject or missed reject.
- if (promise.isHandled(globalThis.vm())) { + if (promise.status(globalThis.vm()) != .pending) { return; }
🧹 Nitpick comments (6)
test/js/web/fetch/response-cyclic-reference.test.ts (1)
17-20: Make heapStats access robustUse optional chaining to avoid a crash if objectTypeCounts is undefined.
- expect(heapStats().objectTypeCounts.ReadableStream || 0).toBeLessThanOrEqual(4); + expect(heapStats().objectTypeCounts?.ReadableStream ?? 0).toBeLessThanOrEqual(4);test/js/web/fetch/request-cyclic-reference.test.ts (1)
17-20: Make heapStats access robustSame as the response test: guard access to avoid undefined property crashes.
- expect(heapStats().objectTypeCounts.ReadableStream || 0).toBeLessThanOrEqual(4); + expect(heapStats().objectTypeCounts?.ReadableStream ?? 0).toBeLessThanOrEqual(4);Optionally rename local “response” (Line 9) to “request” for clarity.
src/bun.js/api/server.zig (1)
1189-1195: Standardize Request init API usageThis uses Request.init2 while other sites use Request.init wrapped in Request.new. Consider unifying (single init name/signature) to avoid confusion and reduce divergent code paths.
src/bun.js/webcore/Body.zig (2)
1112-1113: Make debug log non-verbose by defaultUse Output.scoped(..., false) per project guidance to avoid noisy logs.
As per coding guidelines
- const log = Output.scoped(.BodyMixin, .visible); + const log = Output.scoped(.BodyMixin, false);
1232-1232: Gate verbose loggetArrayBuffer()’s log fires on hot path. Either keep logger disabled (scoped(..., false)) or remove this line.
src/bun.js/webcore/Response.zig (1)
658-661: Redundant re-init of #js_reftoJS() already sets #js_ref and calls checkBodyStreamRef. Re-assigning #js_ref is unnecessary.
- const js_value = response.toJS(globalThis); - response.#js_ref = .initWeak(js_value); - return js_value; + return response.toJS(globalThis);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
src/bun.js/api/html_rewriter.zig(10 hunks)src/bun.js/api/server.zig(4 hunks)src/bun.js/api/server/RequestContext.zig(14 hunks)src/bun.js/bindings/JSBakeResponse.cpp(3 hunks)src/bun.js/bindings/JSGlobalObject.zig(1 hunks)src/bun.js/node/types.zig(2 hunks)src/bun.js/webcore/BakeResponse.zig(5 hunks)src/bun.js/webcore/Blob.zig(3 hunks)src/bun.js/webcore/Body.zig(18 hunks)src/bun.js/webcore/Request.zig(24 hunks)src/bun.js/webcore/Response.zig(27 hunks)src/bun.js/webcore/fetch.zig(10 hunks)src/bun.js/webcore/response.classes.ts(4 hunks)test/js/web/fetch/request-cyclic-reference.test.ts(1 hunks)test/js/web/fetch/response-cyclic-reference.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bun.js/node/types.zig
- src/bun.js/webcore/Blob.zig
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/JSBakeResponse.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/JSBakeResponse.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/JSBakeResponse.cpp
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/webcore/Body.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/webcore/Body.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/webcore/Body.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
test/js/web/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Web API tests under test/js/web/
Files:
test/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.ts
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/webcore/response.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/webcore/response.classes.ts
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/RequestContext.zig
🧬 Code graph analysis (3)
src/bun.js/bindings/JSBakeResponse.cpp (1)
src/bun.js/bindings/bindings.cpp (9)
void(3111-3117)void(3119-3125)void(6476-6479)void(6481-6485)void(6571-6578)void(6580-6583)void(6721-6763)globalObject(2561-2565)globalObject(2561-2561)
test/js/web/fetch/response-cyclic-reference.test.ts (1)
test/js/node/http2/node-http2-memory-leak.js (1)
heapStats(8-8)
test/js/web/fetch/request-cyclic-reference.test.ts (1)
test/js/node/http2/node-http2-memory-leak.js (1)
heapStats(8-8)
🔇 Additional comments (20)
src/bun.js/bindings/JSGlobalObject.zig (1)
904-908: Direct stream access for Locked bodies is correct. It attempts JS-origin streams first viagetBodyReadableStreamand preserves the existing blob-to-stream fallback when null.src/bun.js/webcore/response.classes.ts (1)
7-8: Confirm API surface for “values: ['stream']” and bun-typesConstructors needing this look good. Verify that exposing values: ["stream"] for Request/Response doesn’t unintentionally surface a public .stream or alter d.ts expectations. Update bun-types snapshots if needed.
Would you confirm codegen keeps “stream” internal and adjust bun-types tests if they expect no such field?
Also applies to: 16-17, 73-74, 91-92
src/bun.js/bindings/JSBakeResponse.cpp (1)
36-36: Signature plumbed correctly; verify ABI/linkage across targetsThe extra js_this (EncodedJSValue) is passed consistently. Looks good. Please confirm Zig export matches exactly and that Windows SYSV_ABI is correct in all toolchains.
Also applies to: 215-217, 246-246
src/bun.js/api/server.zig (1)
1231-1233: Good: use Response.setUrl instead of direct fieldSetter aligns with the new encapsulation.
src/bun.js/webcore/BakeResponse.zig (2)
21-23: js_this threading and constructor update look correctForwarding js_this to Response.constructor matches constructNeedsThis. No issues spotted.
Also applies to: 31-48
107-121: Response.init usage LGTMInitialization via Response.init (status/headers/body/url/redirected) is consistent with the new API.
src/bun.js/api/server/RequestContext.zig (4)
680-685: Abort path: detaching/aborting body stream is correctUsing getBodyReadableStream + detachReadableStream + abort is the right lifecycle on abort.
1214-1221: Locking bodyValue with ReadableStreamAssigning bodyValue = .Locked with a strong ReadableStream ref ensures GC safety while streaming. Looks good.
1800-1844: doRenderWithBody signature and call site LGTMPassing owned_readable and using response.getBodyValue()/getBodyReadableStream at call site is consistent with the new API.
Also applies to: 1999-2004
2230-2234: Header accessor refactor LGTMgetInitHeaders/swapInitHeaders integration and write path via writeHeaders are correct and reduce direct struct access.
Also applies to: 2237-2248
src/bun.js/api/html_rewriter.zig (1)
202-210: Response API migration + owned stream propagation looks correctInit/headers/url routed via new accessors; body length/readable stream usage aligns with Body.ValueBufferer.run.
Also applies to: 421-480, 490-499
src/bun.js/webcore/Response.zig (1)
44-87: Private-field encapsulation looks consistentsetInit/setInitHeaders/getters and formatting now use #init/#body/#url/#redirected consistently.
Also applies to: 99-107, 250-306
src/bun.js/webcore/Request.zig (8)
5-14: LGTM! Private field refactoring aligns with Zig conventions.The use of
#prefix for private fields (#headers,#body,#js_ref) follows Zig best practices. The new#js_refweak reference prevents cycles between JS and Zig objects, and thehttpsfield supports protocol tracking.
194-221: Stream lifecycle helpers look correct.The new
getBodyReadableStreamanddetachReadableStreamhelpers properly manage the stream lifecycle by checking both#js_ref(viajs.gc.stream) andLocked.readable. The dual-source approach supports the ownership transfer pattern implemented incheckBodyStreamRef.
386-409: LGTM! Finalization sequence is correct.The finalization order is proper:
#js_ref.finalize()- clears JS referencefinalizeWithoutDeinit()- derefs headers, url, signal#body.unref()- releases body reference- Conditional
bun.destroy(this)based on weak pointerThis ordering prevents use-after-free and ensures all references are properly released.
589-606: LGTM! Weak reference initialization prevents cycles.The initialization of
#js_refwith.initWeak(this_value)(line 595) prevents cyclic references between the Request object and its JavaScript wrapper. The defer cleanup logic (lines 597-605) correctly handles the case where the body is replaced during construction by comparing pointers at line 602.
831-840: Content-Type header logic is correct.The conditional logic properly sets the Content-Type header from the blob only when:
- Body is a Blob with non-empty content_type
- Headers exist
- ContentType header not already set
This prevents overwriting existing headers while ensuring blob content types are propagated.
860-885: LGTM! Stream handling in doClone is correct.The cloning logic properly handles teed ReadableStreams (lines 870-881):
- Updates the cached
.bodyvalue on both the cloned and original requests to point to their respective streams- Calls
checkBodyStreamRefto ensure ownership is transferred tojs.gc.streamThis ensures both the original and cloned requests maintain correct references to their teed streams.
1002-1013: LGTM! Stream-aware cloning logic is correct.The
cloneIntofunction properly handles ReadableStreams:
- Checks
#js_refviajs.gc.streamfor cached stream (lines 1003-1010)- Uses
cloneWithReadableStreamif a stream exists, which properly tees the stream- Falls back to standard
cloneif no stream is present (line 1012)This ensures ReadableStreams are properly teed during cloning while avoiding unnecessary work when no stream exists.
5-1075: Well-executed refactoring to isolate body usage.This refactoring systematically moves body and headers access behind private fields and accessor APIs, which supports the PR objective of isolating Response body usage. Key improvements:
- Private fields: Using
#prefix per Zig convention for#headers,#body,#js_ref- Cycle prevention: Weak
#js_refand stream ownership transfer tojs.gc.streamprevent cyclic references- Stream lifecycle: New helpers (
getBodyReadableStream,detachReadableStream,checkBodyStreamRef) manage stream ownership correctly- Memory safety: Proper ref/unref sequences in finalize and cleanup paths
The refactoring is consistent throughout and appears to maintain correctness while improving encapsulation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/webcore/Body.zig (1)
1642-1660: Fix AnyPromise.status enum casing to match RequestContext usage.Elsewhere you use .pending/.fulfilled/.rejected. Here it’s .Pending/.Fulfilled/.Rejected, which likely won’t compile.
Apply this patch:
- switch (promise.status(globalThis.vm())) { - .Pending => { + switch (promise.status(globalThis.vm())) { + .pending => { assignment_result.then( globalThis, sink, onResolveStream, onRejectStream, ); }, - .Fulfilled => { + .fulfilled => { defer stream.value.unprotect(); sink.handleResolveStream(false); }, - .Rejected => { + .rejected => { defer stream.value.unprotect(); sink.handleRejectStream(promise.result(globalThis.vm()), false); }, }
♻️ Duplicate comments (2)
src/bun.js/api/server/RequestContext.zig (1)
1204-1213: Inconsistent AnyPromise status enum casing (compile-time break).Here you switch on promise.status(globalThis.vm()) using .pending/.fulfilled/.rejected. In Body.ValueBufferer.createJSSink the code uses .Pending/.Fulfilled/.Rejected. Use one enum shape consistently for AnyPromise.status; the lowercase variants are used elsewhere in this file.
Apply the same lowercase in Body.ValueBufferer to avoid build break. See follow-up diff in Body.zig comment.
src/bun.js/webcore/Request.zig (1)
129-142: Remove the TODO comment and clarify init2's purpose.Per the previous review,
init2is actively used insrc/bun.js/api/server.zig:1189and must be retained. The TODO "do we need this?" should be removed.Replace the TODO with a descriptive comment explaining that
init2is a specialized constructor invoked by the server API when constructing Requests with nullable headers and a HiveRef body, clarifying why it differs from the standardinitfunction.
🧹 Nitpick comments (1)
src/bun.js/webcore/fetch.zig (1)
444-473: Double-check buffer lifetime when sending final chunk (.temporary_and_done).Here, after detachReadableStream(), you pass scheduled_response_buffer.items as .temporary_and_done to readable.ptr.Bytes.onData(). Since buffer_reset defaults to true (and isn’t flipped here), the defer will reset the buffer at function end. If the sink doesn’t synchronously copy, this risks use-after-free. Earlier path uses .owned_and_done with buffer_reset = false.
Recommend aligning with the owned pattern at end-of-stream:
- set buffer_reset = false
- discard allocatedSlice()
- pass .owned_and_done = bun.ByteList.moveFromList(scheduled_response_buffer)
This keeps ownership/lifetime explicit and consistent with the other branch. Please confirm the ByteStream contract if you prefer temporary_and_done here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/bun.js/api/server/RequestContext.zig(23 hunks)src/bun.js/webcore/Body.zig(19 hunks)src/bun.js/webcore/Request.zig(24 hunks)src/bun.js/webcore/fetch.zig(10 hunks)test/js/web/fetch/request-cyclic-reference.test.ts(1 hunks)test/js/web/fetch/response-cyclic-reference.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/js/web/fetch/request-cyclic-reference.test.ts
- test/js/web/fetch/response-cyclic-reference.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/bun.js/webcore/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/fetch.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/webcore/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/fetch.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/fetch.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.zig
🔇 Additional comments (12)
src/bun.js/webcore/fetch.zig (2)
958-969: Init/accessor refactor looks good.Response.init(...) and response.getBodyValue()/getBodyReadableStream adoption is consistent and improves encapsulation.
Also applies to: 1424-1434, 2352-2360
994-1021: Finalizer logic: verify “promise present” condition.Early-return for streaming or non-locked is fine. For buffered (.Locked without readable_stream_ref), you keep loading if Locked.promise exists, and ignore if it doesn’t. That matches the comment (2b vs 2a). Please confirm Locked.promise cannot be a non-empty sentinel without a real Promise.
src/bun.js/api/server/RequestContext.zig (2)
680-685: ReadableStream lifetime handling looks solid.
- Abort path detaches and aborts the response’s stream via accessors.
- finalizeWithoutDeinit and doRender paths deinit the strong ref appropriately.
No issues spotted.
Also applies to: 755-755, 2007-2008
1456-1507: Accessor migration (getBodyValue/getBodyReadableStream) is correct.Switching to bodyValue-based logic and updating headers via accessors keeps internals encapsulated and consistent with the new API.
Also applies to: 1561-1577, 1619-1636, 2180-2196
src/bun.js/webcore/Body.zig (2)
25-29: ReadableStream ownership propagation is well-structured.
- cloneWithReadableStream/tee and Mixin methods properly pass an owned readable and keep Strong refs where needed.
- ValueBufferer’s bufferLockedBodyValue holds a Strong ref to prevent premature GC.
Looks good.
Also applies to: 958-975, 1042-1079, 1477-1517, 1668-1737
1116-1141: Nice: body getters respect host-owned ReadableStream when present.Using getBodyReadableStream to avoid creating new streams and passing it into setPromise reduces churn and prevents TDZ/GC issues.
Also applies to: 1168-1168, 1224-1252, 1261-1287, 1296-1324, 1354-1416
src/bun.js/webcore/Request.zig (6)
7-10: LGTM! Private field declarations follow Zig conventions.The conversion to private fields using the
#prefix aligns with the Zig coding guideline. The new#js_reffield properly integrates with the GC system to track the JS wrapper and manage stream ownership.As per coding guidelines.
194-221: LGTM! Stream accessors implement correct ownership semantics.The helpers properly prioritize
js.gc.streamas the source of truth (line 200-201) and ensure cleanup in both storage locations (lines 214, 217-220). Error handling viatakeExceptionis appropriate.
588-604: LGTM! Constructor initialization and cleanup properly managed.The
constructIntofunction correctly initializes#js_refwith a weak reference (line 594) and uses defer/errdefer blocks to ensure proper cleanup of resources (lines 596-604). The cleanup logic properly handles the case where#bodyis replaced during construction (lines 601-603).
991-1031: LGTM! Clone logic correctly handles stream extraction.The
cloneIntofunction properly extracts readable streams from the JS reference before cloning (lines 1002-1009). The fallback to regularclonewhen no stream is present (line 1011) is appropriate. Error handling viaerrdeferensures cleanup of partially constructed state (lines 1013, 1016, 1018).
853-857: LGTM! Body accessor correctly returns private field.The
getBodyValuefunction correctly returns a reference to#body.value, maintaining the established accessor pattern throughout this refactoring.
887-939: LGTM! Header accessors maintain proper encapsulation.The header accessor functions (
hasFetchHeaders,setFetchHeaders,ensureFetchHeaders,getFetchHeadersUnlessEmpty,getFetchHeaders) correctly use the private#headersfield while preserving the original logic. The lazy initialization pattern inensureFetchHeaders(lines 911-938) properly creates headers from the request context or body content-type when needed.
What does this PR do?
Just a refactoring isolating the usage of Response body this is the first step necessary for #23319
How did you verify your code works?
CI