Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
56 changes: 16 additions & 40 deletions src/runtime/api/bun/h2_frame_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1116,11 +1116,10 @@
// `WebWorker::destroy` has raw-deallocated the VM, so `HiveArray::Drop`
// on any leaked parser would touch freed JSC/uws state. Skip slot
// teardown (a leaked parser is a bug anyway) while still freeing the
// pool allocation itself.
static POOL: RefCell<Option<Box<ManuallyDrop<H2FrameParserHiveAllocator>>>> =
const { RefCell::new(None) };
static SHARED_REQUEST_BUFFER: RefCell<Box<[u8; 16384]>> = RefCell::new(Box::new([0u8; 16384]));
}

Check warning on line 1122 in src/runtime/api/bun/h2_frame_parser.rs

View check run for this annotation

Claude / Claude Code Review

Stale comment: 'these two buffers are 32 KB combined' after SHARED_REQUEST_BUFFER removal

nit: the leading comment on this `thread_local!` block ("these two buffers are 32 KB combined and would otherwise dominate PT_TLS MemSiz…") described `CORK_BUFFER` (16 KB) + `SHARED_REQUEST_BUFFER` (16 KB). This PR deletes `SHARED_REQUEST_BUFFER`, leaving only the one 16 KB `CORK_BUFFER`, so the comment is now factually wrong — worth updating to something like "Boxed so only a pointer lives in static TLS — a 16 KB buffer would otherwise dominate PT_TLS MemSiz…" in the same change that invalidate
Comment thread
robobun marked this conversation as resolved.

/// One wire-order piece of a multi-frame send_data batch (see BATCH_SEGMENTS).
#[derive(Clone, Copy)]
Expand Down Expand Up @@ -1738,19 +1737,7 @@
};
let _ = data_header.write(&mut writer);
if padding != 0 {
break 'brk SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — use ptr::copy (memmove)
unsafe {
core::ptr::copy(
able_to_send.as_ptr(),
buffer.as_mut_ptr().add(1),
able_to_send.len(),
);
}
buffer[0] = padding;
buffer[1 + able_to_send.len()..payload_size].fill(0);
writer.write_all(&buffer[0..payload_size]).is_ok()
});
break 'brk writer.write_padded(&able_to_send, padding).is_ok();
} else {
break 'brk writer.write_all(&able_to_send).is_ok();
}
Expand Down Expand Up @@ -1802,19 +1789,7 @@
};
let _ = data_header.write(&mut writer);
if padding != 0 {
break 'brk SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — ptr::copy is memmove; dst capacity covers payload_size
unsafe {
core::ptr::copy(
frame_slice.as_ptr(),
buffer.as_mut_ptr().add(1),
frame_slice.len(),
);
}
buffer[0] = padding;
buffer[1 + frame_slice.len()..payload_size].fill(0);
writer.write_all(&buffer[0..payload_size]).is_ok()
});
break 'brk writer.write_padded(frame_slice, padding).is_ok();
} else {
break 'brk writer.write_all(frame_slice).is_ok();
}
Expand Down Expand Up @@ -6165,6 +6140,19 @@
}
}

impl DirectWriterStruct {
/// The payload of a PADDED DATA frame (RFC 9113 6.1); the caller wrote the frame header.
fn write_padded(&mut self, data: &[u8], padding: u8) -> bun_io::Result<()> {
// Owned per call: over a JS-backed transport, `write()` can re-enter
// send_data()/flush_queue() before this frame is fully corked.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut payload = Vec::with_capacity(1 + data.len() + padding as usize);
payload.push(padding);
payload.extend_from_slice(data);
payload.resize(payload.len() + padding as usize, 0);
self.write_all(&payload)
}

Check warning on line 6153 in src/runtime/api/bun/h2_frame_parser.rs

View check run for this annotation

Claude / Claude Code Review

PR description describes superseded Cell<Option<Box<[u8]>>> take/put-back design, not the shipped per-call Vec

The **Fix** section of the PR description still describes the superseded intermediate design from f5c28aa2 — "moves the scratch buffer out of its thread-local slot (`Cell<Option<Box<[u8]>>>`, take / put back)" — but the shipped `write_padded` (5fc4e380 → 54a73266) allocates a fresh `Vec` per call and deletes `SHARED_REQUEST_BUFFER` entirely; there is no thread-local slot or take/put-back anymore. Since the description becomes the squash-commit message (per `.claude/docs/landing-prs.md` § PR proc
Comment thread
claude[bot] marked this conversation as resolved.
}

// ──────────────────────────────────────────────────────────────────────────
// H2FrameParser impl — JS host fns (part 1)
// ──────────────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -7294,19 +7282,7 @@
let mut writer = self.to_writer();
let _ = data_header.write(&mut writer);
if padding != 0 {
SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — ptr::copy is memmove; dst capacity covers payload_size
unsafe {
core::ptr::copy(
slice.as_ptr(),
buffer.as_mut_ptr().add(1),
slice.len(),
);
}
buffer[0] = padding;
buffer[1 + slice.len()..payload_size].fill(0);
let _ = writer.write_all(&buffer[0..payload_size]);
});
let _ = writer.write_padded(slice, padding);
} else {
let _ = writer.write_all(slice);
}
Expand Down
72 changes: 72 additions & 0 deletions test/js/node/http2/node-http2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,78 @@ it("http2 session.goaway() opaqueData survives re-entrant buffer detach over a J
expect(exitCode).toBe(0);
}, 15_000);

it("http2 padded DATA write survives a re-entrant stream write from a JS Duplex _write", async () => {
// With padding enabled, a single-frame DATA write that crosses the 16 KiB cork boundary
// flushes the cork into the JS transport mid-frame (onWrite → Duplex _write). A transport
// that writes to another padded stream from inside its _write re-enters the same padded
// send path while the outer frame is still being assembled. That must neither abort the
// process nor clobber the outer frame's bytes: every payload byte of all three writes has
// to reach the transport (same totals as node).
const fixture = `
const http2 = require("node:http2");
const { Duplex } = require("node:stream");
// fill: 13000 -> 9+13256 corked; outer: 12000 -> 9+12256, crosses the cork boundary;
// side: 3000 -> 9+3256, written from inside the transport _write during that flush.
const EXPECTED_WIRE = 9 + 13256 + 9 + 12256 + 9 + 3256;
let side, armed = false, reentered = false, total = 0, onComplete;
const counts = { 0x41: 0, 0x42: 0, 0x43: 0 };
const fail = what => err => {
console.error(what, err);
process.exit(1);
};
const duplex = new Duplex({
read() {},
write(chunk, enc, cb) {
if (armed) {
for (const b of chunk) if (b in counts) counts[b]++;
total += chunk.length;
// The first chunk carrying outer's payload is the mid-frame cork flush.
if (!reentered && chunk.includes(0x41)) {
reentered = true;
side.write(Buffer.alloc(3000, 0x42));
}
if (total >= EXPECTED_WIRE && onComplete) onComplete();
}
cb();
},
});
const session = http2.connect("http://127.0.0.1:1", {
createConnection: () => duplex,
paddingStrategy: http2.constants.PADDING_STRATEGY_MAX,
});
session.on("error", fail("session error"));
await new Promise(r => session.once("connect", r));
const frame = (type, flags) => Buffer.from([0, 0, 0, type, flags, 0, 0, 0, 0]);
// server preface by hand: empty SETTINGS, then the ACK for the client's SETTINGS
duplex.push(Buffer.concat([frame(4, 0), frame(4, 1)]));
await new Promise(r => setImmediate(r));
side = session.request({ ":method": "POST", ":path": "/side" }, { endStream: false });
const outer = session.request({ ":method": "POST", ":path": "/outer" }, { endStream: false });
const fill = session.request({ ":method": "POST", ":path": "/fill" }, { endStream: false });
for (const s of [side, outer, fill]) s.on("error", fail("stream error"));
// The three HEADERS frames leave the cork in this tick's deferred auto-flush, which runs
// before the loop reaches the next immediate.
await new Promise(r => setImmediate(r));
const complete = new Promise(r => (onComplete = r));
fill.write(Buffer.alloc(13000, 0x43));
armed = true;
outer.write(Buffer.alloc(12000, 0x41));
if (total < EXPECTED_WIRE) await complete;
console.log(JSON.stringify({ reentered, total, A: counts[0x41], B: counts[0x42], C: counts[0x43] }));
process.exit(0);
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
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(JSON.parse(stdout.trim())).toEqual({ reentered: true, total: 28795, A: 12000, B: 3000, C: 13000 });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(exitCode).toBe(0);
});

it("http2 server sends protocol-error GOAWAY on stream 0", async () => {
// RFC 9113 section 6.8: GOAWAY frames MUST be sent with a stream identifier
// of 0 in the frame header; the last processed stream id lives in the
Expand Down
Loading