streams: drop dead ended = false in NetworkSink and FetchRequestBodySink start - #37406
Conversation
…ink start Both start() methods return early when ended is set, so the assignment that followed could only ever write false over false.
|
Warning Review limit reached
Next review available in: 22 seconds 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 (2)
Comment |
There was a problem hiding this comment.
LGTM — both removed assignments are provably dead.
What was reviewed:
- Confirmed both
start()methods early-return whenself.endedis true, so the removedself.ended = falsecould only ever writefalseoverfalse. - Checked that
endeddefaults tofalsein both structs, so no first-call initialization is lost. - No other callers or side effects touched.
Extended reasoning...
Overview
This PR removes one dead line from each of NetworkSink::start (src/runtime/webcore/streams.rs) and FetchRequestBodySink::start (src/runtime/webcore/fetch/FetchRequestBodySink.rs). Both functions begin with if self.ended { return Ok(()); }, so control flow can only reach the removed self.ended = false; when self.ended is already false. The assignment is a tautological no-op.
Security risks
None. This is pure dead-code removal in stream sink initialization; no input handling, allocation, or lifecycle logic is changed.
Level of scrutiny
Very low. The change is two deleted lines whose deadness is locally provable from the guard three lines above each. ended is initialized to false in Default::default() for both structs, so there is no scenario where this assignment did useful first-time initialization. The PR description accurately characterizes the change and references #36770 as the broader lifecycle rework this trims ahead of.
Other factors
The author ran the relevant fetch/S3 stream test suites. No new tests are needed since there is no observable behavior to assert — this is exactly the "delete dead code in the same PR that makes it dead" pattern the review guidelines call for. No prior reviewer comments to address.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
What
NetworkSink::startandFetchRequestBodySink::startboth return early whenself.endedis set and then assignedself.ended = falsea few lines later, so the assignment could only ever writefalseoverfalse. Removed in both places. No behaviour change.Related: #36770 reworks
NetworkSink's lifecycle flags more broadly; this is just the two dead lines.Verification
bun bd test test/js/bun/s3/s3.test.ts test/js/web/fetch/fetch.stream.test.ts test/js/web/fetch/body.test.ts test/js/web/fetch/fetch-abort-stream-body.test.tspasses (the S3 cases that run against a local server included).