Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/runtime/webcore/ByteStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,16 +305,16 @@ impl ByteStream {
}

if self.buffer_action.get().is_some() {
self.signal_drained();
if let streams::Result::Err(err) = &stream {
// Explicit post-reject cleanup; runs after `action.reject`
// (`?` would skip it).
bun_output::scoped_log!(ByteStream, "ByteStream.onData err action.reject()");

let global = self.parent_const().global_this();
// R-2: move the action out of the cell *before* calling
// `reject` (which resolves a JS promise and may re-enter).
// R-2: move the action out of the cell *before* `signal_drained`
// and `reject`; both can re-enter and consume the slot.
Comment thread
robobun marked this conversation as resolved.
let mut action = self.buffer_action.replace(None).unwrap();
self.signal_drained();
let res = action.reject(global, err);

self.buffer.with_mut(|b| {
Expand All @@ -330,9 +330,16 @@ impl ByteStream {
return res;
}

// R-2: the drain signal can re-enter and consume `buffer_action`,
// so the paths below re-take it with `let`-`else`.
Comment thread
robobun marked this conversation as resolved.
self.signal_drained();

if self.has_received_last_chunk.get() {
// `defer { this.buffer_action = null; }` — handled by `replace(None)` below.
let mut action = self.buffer_action.replace(None).unwrap();
let Some(mut action) = self.buffer_action.replace(None) else {
// Consumed re-entrantly during `signal_drained`.
return Ok(());
};

if self.buffer.get().capacity() == 0 && matches!(stream, streams::Result::Done) {
bun_output::scoped_log!(
Expand Down
81 changes: 81 additions & 0 deletions test/js/web/fetch/fetch-abort-parked-reads-fixture.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 23 additions & 2 deletions test/js/web/fetch/fetch.stream.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Socket } from "bun";
import { describe, expect, it } from "bun:test";
import { describe, expect, it, test } from "bun:test";
import { createReadStream, readFileSync } from "fs";
import { gcTick, isWindows, tempDirWithFilesAnon } from "harness";
import { bunEnv, bunExe, gcTick, isWindows, tempDirWithFilesAnon } from "harness";
import http from "http";
import type { AddressInfo } from "net";
import path, { join } from "path";
Expand Down Expand Up @@ -1422,3 +1422,24 @@ describe.concurrent("fetch() with streaming", () => {
server.kill("SIGTERM");
});
});

// ByteStream::on_data used to call signal_drained() before taking the pending
// buffer action out of its cell; the drain signal can re-enter and consume the
// action, so the unwrap() that followed panicked and killed the process
// (seen as a crash when aborting fetches with parked reads on streaming
// bodies). The race is timing-dependent, so this stress fixture exercises the
// abort paths and asserts every parked consumer settles with exit code 0.
test.concurrent("aborting streaming fetches with parked body consumers settles them without crashing", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), join(import.meta.dir, "fetch-abort-parked-reads-fixture.ts")],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout).toBe("done 12\n");
expect(exitCode).toBe(0);
});
Loading