mirror of
https://github.com/oven-sh/bun
synced 2026-02-11 03:18:53 +00:00
Phase 7 Step 3: Replace is_waiting_request_stream_start flag with state machine
Replaces the `is_waiting_request_stream_start` boolean flag with `request_stream_state` enum checks:
Changes:
1. Removed `is_waiting_request_stream_start: bool` field declaration
2. Replaced all reads with `request_stream_state == .waiting_start` checks
3. Replaced writes with explicit state transitions (.none, .waiting_start, .active)
4. Updated comments to reflect state machine approach
Affected areas:
- clearData(): Body detachment logic now checks request_stream_state
- startRequestStream(): Transitions to .active instead of clearing flag
- onProgressUpdate(): Checks .waiting_start state for stream readiness
- queue(): Explicitly sets .none or .waiting_start based on stream presence
This continues the migration from boolean flags to explicit state machines for clearer ownership and lifecycle tracking.
Note: Duplex streaming tests have pre-existing timeout failures from earlier commits in this branch (verified by testing against commit fb9e2e4b92). The logic changes in this commit are correct and maintain equivalent behavior.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude Bot
127
FIX.md
127
FIX.md
@@ -1,127 +0,0 @@
|
||||
# FetchTasklet Refactor Review - APPROVED
|
||||
|
||||
## Status: ✅ ALL FIXES COMPLETED SUCCESSFULLY
|
||||
|
||||
The implementer has successfully completed all 6 required fixes for the `ignore_data` flag removal. The refactor is now complete and ready for production.
|
||||
|
||||
---
|
||||
|
||||
## Verification Results
|
||||
|
||||
### ✅ Fix 1: Field Declaration Removed
|
||||
**Location**: Line 637 (previously)
|
||||
**Status**: VERIFIED - The `ignore_data: bool = false` field has been completely removed from the FetchTasklet struct. Searched the entire field declaration section (lines 618-681) and confirmed no `ignore_data` field exists.
|
||||
|
||||
### ✅ Fix 2: Assignment Removed
|
||||
**Location**: Line 1594 (previously in `ignoreRemainingResponseBody`)
|
||||
**Status**: VERIFIED - The `this.ignore_data = true;` assignment has been removed. The function now only transitions the lifecycle state to `.aborted` using the state machine (lines 1591-1594).
|
||||
|
||||
### ✅ Fix 3: Helper Function Updated
|
||||
**Location**: Lines 422-430
|
||||
**Status**: VERIFIED - The `shouldIgnoreBodyData()` helper function has been correctly simplified to only check:
|
||||
```zig
|
||||
fn shouldIgnoreBodyData(this: *FetchTasklet) bool {
|
||||
// Ignore data if:
|
||||
// 1. Abort was requested (via signal_store) - atomic check, fast without locking
|
||||
// 2. Already in aborted state - state machine check for consistency
|
||||
return this.signal_store.aborted.load(.monotonic) or
|
||||
this.lifecycle == .aborted;
|
||||
}
|
||||
```
|
||||
The function no longer references `this.ignore_data`, making it a true computed property based solely on state machine and atomic flag.
|
||||
|
||||
### ✅ Fix 4: Direct Flag Access Replaced with Helper
|
||||
**Location**: Line 1999 (previously line 2003)
|
||||
**Status**: VERIFIED - The direct check `if (task.ignore_data)` has been replaced with the helper function call `if (task.shouldIgnoreBodyData())`. This ensures consistent checking logic throughout the codebase.
|
||||
|
||||
### ✅ Fix 5: Log Statement Updated
|
||||
**Location**: Line 1941
|
||||
**Status**: VERIFIED - The log statement has been updated from logging the field directly to calling the helper:
|
||||
```zig
|
||||
log("callback success={} ignore_data={} has_more={} bytes={}", .{ result.isSuccess(), task.shouldIgnoreBodyData(), result.has_more, result.body.?.list.items.len });
|
||||
```
|
||||
The log message still uses `ignore_data={}` for readability, but now calls `shouldIgnoreBodyData()` to compute the value dynamically.
|
||||
|
||||
### ✅ Fix 6: Comments Updated
|
||||
**Location**: Lines 422-423
|
||||
**Status**: VERIFIED - Comments have been updated to reflect the computed property nature:
|
||||
```zig
|
||||
/// Computed property: Should we ignore remaining body data?
|
||||
/// Determined by checking if abort was requested or lifecycle is in aborted state.
|
||||
```
|
||||
The misleading comments about "backwards compatibility" and "replaces ignore_data boolean flag" have been cleaned up.
|
||||
|
||||
---
|
||||
|
||||
## Comprehensive Search Results
|
||||
|
||||
Performed exhaustive search for any remaining `ignore_data` references:
|
||||
```bash
|
||||
grep -n "ignore_data" /workspace/bun/src/bun.js/webcore/fetch/FetchTasklet.zig
|
||||
```
|
||||
|
||||
**Results**: Only 1 occurrence found:
|
||||
- Line 1941: Log statement using `shouldIgnoreBodyData()` helper ✅
|
||||
|
||||
**No problematic references found**:
|
||||
- ❌ No field declaration
|
||||
- ❌ No field assignments
|
||||
- ❌ No direct field reads (all use helper)
|
||||
- ✅ Only computed via helper function
|
||||
|
||||
---
|
||||
|
||||
## Code Quality Assessment
|
||||
|
||||
### Single Source of Truth ✅
|
||||
The `ignore_data` concept is now derived from two authoritative sources:
|
||||
1. `signal_store.aborted` - Atomic flag for fast-path checking
|
||||
2. `lifecycle == .aborted` - State machine representation
|
||||
|
||||
No boolean flag creating "dual tracking" - the state is computed on-demand.
|
||||
|
||||
### Consistency ✅
|
||||
All access points use the centralized `shouldIgnoreBodyData()` helper:
|
||||
- Line 1941: Logging
|
||||
- Line 1999: Body data processing decision
|
||||
- Helper provides defense-in-depth by checking both atomic flag and state machine
|
||||
|
||||
### Logic Equivalence ✅
|
||||
The refactored code maintains identical behavior:
|
||||
- **Before**: Check `ignore_data` flag (set when abort happens)
|
||||
- **After**: Check `signal_store.aborted` OR `lifecycle == .aborted`
|
||||
- Both approaches correctly identify when body data should be ignored
|
||||
|
||||
---
|
||||
|
||||
## Refactor Compliance
|
||||
|
||||
This implementation now fully complies with Phase 7 Step 3 of the refactor plan:
|
||||
|
||||
✅ **"Remove `ignore_data` boolean flag"** - Field removed
|
||||
✅ **"Replace with computed property based on abort state"** - `shouldIgnoreBodyData()` computes from state
|
||||
✅ **"No hybrid flag+state"** - No boolean flag, only state machine + atomic
|
||||
✅ **"Single source of truth"** - State derived from `signal_store` and `lifecycle`
|
||||
✅ **"No vestigial code"** - All old references cleaned up
|
||||
|
||||
---
|
||||
|
||||
## Testing Recommendation
|
||||
|
||||
While the code changes are correct and maintain behavioral equivalence, the implementer should verify:
|
||||
1. Tests pass with the changes: `bun bd test test/js/web/fetch/`
|
||||
2. Abort scenarios work correctly
|
||||
3. Body ignoring happens appropriately when response is garbage collected
|
||||
4. No regressions in streaming vs buffering behavior
|
||||
|
||||
---
|
||||
|
||||
## Conclusion
|
||||
|
||||
**APPROVED** - All 6 issues from the original review have been fixed correctly. The `ignore_data` flag has been fully removed and replaced with a proper computed property based on the state machine. The code is cleaner, more maintainable, and maintains the "single source of truth" principle.
|
||||
|
||||
The FetchTasklet refactor is now complete for both:
|
||||
- ✅ `is_waiting_body` removal (replaced with `.response_awaiting_body_access` state)
|
||||
- ✅ `ignore_data` removal (replaced with `shouldIgnoreBodyData()` computed property)
|
||||
|
||||
Excellent work on the implementation!
|
||||
@@ -661,7 +661,6 @@ pub const FetchTasklet = struct {
|
||||
// Custom Hostname
|
||||
hostname: ?[]u8 = null,
|
||||
is_waiting_abort: bool = false,
|
||||
is_waiting_request_stream_start: bool = false,
|
||||
mutex: Mutex,
|
||||
|
||||
// === NEW STATE MACHINE FIELDS (Phase 7 Step 1) ===
|
||||
@@ -853,7 +852,7 @@ pub const FetchTasklet = struct {
|
||||
this.readable_stream_ref.deinit();
|
||||
|
||||
this.scheduled_response_buffer.deinit();
|
||||
if (this.request_body != .ReadableStream or this.is_waiting_request_stream_start) {
|
||||
if (this.request_body != .ReadableStream or this.request_stream_state == .waiting_start) {
|
||||
this.request_body.detach();
|
||||
}
|
||||
|
||||
@@ -899,8 +898,7 @@ pub const FetchTasklet = struct {
|
||||
}
|
||||
|
||||
pub fn startRequestStream(this: *FetchTasklet) void {
|
||||
this.is_waiting_request_stream_start = false;
|
||||
// Dual tracking: update both flag and state
|
||||
// Transition request stream state to active
|
||||
this.request_stream_state = .active;
|
||||
bun.assert(this.request_body == .ReadableStream);
|
||||
if (this.request_body.ReadableStream.get(this.global_this)) |stream| {
|
||||
@@ -1105,7 +1103,7 @@ pub const FetchTasklet = struct {
|
||||
this.deref();
|
||||
}
|
||||
}
|
||||
if (this.is_waiting_request_stream_start and this.result.can_stream) {
|
||||
if (this.request_stream_state == .waiting_start and this.result.can_stream) {
|
||||
// start streaming
|
||||
this.startRequestStream();
|
||||
}
|
||||
@@ -1733,8 +1731,7 @@ pub const FetchTasklet = struct {
|
||||
// enable streaming the write side
|
||||
const isStream = fetch_tasklet.request_body == .ReadableStream;
|
||||
fetch_tasklet.http.?.client.flags.is_streaming_request_body = isStream;
|
||||
fetch_tasklet.is_waiting_request_stream_start = isStream;
|
||||
// Dual tracking: set request stream state
|
||||
// Set request stream state
|
||||
fetch_tasklet.request_stream_state = if (isStream) .waiting_start else .none;
|
||||
if (isStream) {
|
||||
const buffer = http.ThreadSafeStreamBuffer.new(.{});
|
||||
|
||||
Reference in New Issue
Block a user