Read Request's abort signal and JSArrayIterator's fast path through accessors - #37651
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 3:52 PM PT - Aug 11th, 2026
✅ @alii, your commit 04acfa5acd6c9a6f36bbbfd267f417879743a613 passed in 🧪 To try this PR locally: bunx bun-pr 37651That installs a local version of the PR into your bun-37651 --bun |
There was a problem hiding this comment.
LGTM — pure encapsulation refactor, no behavior change.
What was reviewed:
Request.signal→ private +abort_signal()accessor: the sole external reader infetch.rsgets identical semantics (JsCell::get().as_ref()→Option<&AbortSignalRef>, then.ref_()).JSArrayIterator.fast/.array→ private +is_fast():Blob.rsswapsiter.fast.is_none()for!iter.is_fast(), which is definitionally equivalent.- Grepped for other crate-level readers/writers of both fields — none exist outside the owning modules.
Extended reasoning...
Overview
This PR tightens field visibility on two structs and routes their one external reader each through a new accessor:
src/runtime/webcore/Request.rs:signal: JsCell<Option<AbortSignalRef>>goes frompub(crate)to private; newpub(crate) fn abort_signal(&self) -> Option<&AbortSignalRef>returnsself.signal.get().as_ref().src/runtime/webcore/fetch.rs:req.signal.get()→req.abort_signal()at the one call site in'extract_signal. The subsequent.ref_()andNonNull::new(...)are unchanged.src/jsc/JSArrayIterator.rs:arrayandfastgo frompubto private; newpub fn is_fast(&self) -> bool { self.fast.is_some() }.src/runtime/webcore/Blob.rs:iter.fast.is_none()→!iter.is_fast()at the one call site in the parts prescan.
Security risks
None. This is a visibility narrowing with an accessor shim; no new code paths, no input handling changes, no allocation or refcount changes.
Level of scrutiny
Low. The change is mechanical: two pub→private field flips, two one-line accessor methods, and two call-site rewrites that are trivially equivalent by inspection (is_none() ↔ !is_some(); cell.get() pattern-match ↔ cell.get().as_ref()). I confirmed via JsCell::get in src/jsc/JSCell.rs that it returns &T, so self.signal.get().as_ref() on JsCell<Option<AbortSignalRef>> yields Option<&AbortSignalRef> as expected, and AbortSignalRef::ref_() still bumps the C++ refcount identically at the fetch call site.
Other factors
I grepped the workspace for other consumers of JSArrayIterator.{fast,array} and Request.signal:
JSArrayIteratoris used in ~10 files (shell, sql, ConsoleObject, Macro, JSTranspiler, etc.) but only viainit/next/.len/.i— none touch.fastor.array, so making those private compiles cleanly..signal.get()/.signal.set()hits insrc/runtime/server/are onRequestContext.signalor in comments, notRequest.signal. The field's own module (Request.rs) still has full access forconstruct_into/clone_into/get_signal/finalize_without_deinit.
The PR description states cargo check -p bun_jsc -p bun_runtime passes and the debug build was smoke-tested for both affected paths. No new tests are shipped, but per REVIEW.md this is a no-behavior-change encapsulation refactor rather than a behavioral fix, so the existing coverage suffices.
|
I think i've seen this PR 3 times, duplicate flag should find things |
|
Sorry for the noise, there were a few of these in a row today. This one isn't the same change as #37549 (that's fetch's own signal holder in FetchTasklet; this is |
What does this PR do?
Two fields whose owner sets them up under a specific condition were writable from anywhere in the crate:
webcore::Request.signalis only ever set fromconstruct_into(afterAbortSignal::ref_from_jstype-checks it),clone_into, and the lazysignalgetter. Its one outside reader,fetch()extracting the signal from aRequestargument, now goes throughRequest::abort_signal(), and the field is private.JSArrayIterator.fast/.arrayareSome/set only when C++ reported a contiguous butterfly together with thelenthatnext()bounds against;Blob's parts loop readfastto decide whether iteration can run JS. That is nowiter.is_fast(), and both fields are private (i/lenstay public, they are read widely andnext()bounds-checksi).No behavior change.
How did you verify your code works?
cargo check -p bun_jsc -p bun_runtime. Withbun-debug:fetch(new Request(url, { signal }))still rejects withAbortErrorwhen the controller aborts mid-request, andnew Blob([...]).text()is correct for both a plain array (fast path) and one containing an object withtoString(slow path).