mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
fix(http2): prevent extra empty DATA frame on write()+end() pattern (#26410)
## Summary
- Fixes an issue where calling `req.write(data)` followed by `req.end()`
on an HTTP/2 stream would send **three** DATA frames instead of **two**
- This caused AWS ALB and other strict HTTP/2 servers to reject the
connection with `NGHTTP2_FRAME_SIZE_ERROR` (error code 6)
## Root Cause
The `Http2Stream.end()` method was creating an empty buffer
(`Buffer.alloc(0)`) when called without data:
```javascript
if (!chunk) {
chunk = Buffer.alloc(0);
}
return super.end(chunk, encoding, callback);
```
This empty buffer was then passed to the Duplex stream's `end()`, which
triggered `_write()` with the empty buffer before calling `_final()`.
This resulted in:
1. DATA frame with actual data (from `_write`)
2. Empty DATA frame without END_STREAM (from the extra `_write` with
empty buffer)
3. Empty DATA frame with END_STREAM (from `_final`)
The second empty DATA frame was unnecessary and violated some strict
HTTP/2 implementations.
## Fix
Remove the unnecessary empty buffer creation. The Duplex stream's
`end()` method already handles the no-data case correctly by calling
`_final()` directly without calling `_write()`.
## Test plan
- [x] Manually verified with ConnectRPC client and AWS ALB endpoint
- [x] Added regression test
`test/regression/issue/25589-write-end.test.ts`
- [x] Existing HTTP/2 tests pass
- [x] Existing gRPC tests pass
Fixes #25589
🤖 Generated with [Claude Code](https://claude.ai/claude-code)
---------
Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
@@ -1995,10 +1995,10 @@ class Http2Stream extends Duplex {
|
|||||||
typeof callback == "function" && callback();
|
typeof callback == "function" && callback();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
if (!chunk) {
|
|
||||||
chunk = Buffer.alloc(0);
|
|
||||||
}
|
|
||||||
this[bunHTTP2StreamStatus] = status | StreamState.EndedCalled;
|
this[bunHTTP2StreamStatus] = status | StreamState.EndedCalled;
|
||||||
|
// Don't create an empty buffer for end() without data - let the Duplex stream
|
||||||
|
// handle it naturally (just calls _final without _write for empty data).
|
||||||
|
// Creating an empty buffer here causes an extra empty DATA frame to be sent.
|
||||||
return super.end(chunk, encoding, callback);
|
return super.end(chunk, encoding, callback);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
201
test/regression/issue/25589-write-end.test.ts
Normal file
201
test/regression/issue/25589-write-end.test.ts
Normal file
@@ -0,0 +1,201 @@
|
|||||||
|
import { expect, test } from "bun:test";
|
||||||
|
import http2 from "node:http2";
|
||||||
|
|
||||||
|
test("http2 write() + end() pattern should only send two DATA frames (local server)", async () => {
|
||||||
|
// Create a test server that tracks received DATA frames
|
||||||
|
const receivedDataFrames: Array<{ length: number; flags: number }> = [];
|
||||||
|
|
||||||
|
const server = http2.createServer();
|
||||||
|
|
||||||
|
server.on("stream", (stream, headers) => {
|
||||||
|
stream.on("data", chunk => {
|
||||||
|
// Track that we received data
|
||||||
|
receivedDataFrames.push({
|
||||||
|
length: chunk.length,
|
||||||
|
flags: 0, // We can't easily get flags here, but we track frame count
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
stream.on("end", () => {
|
||||||
|
// Send response
|
||||||
|
stream.respond({ ":status": 200 });
|
||||||
|
stream.end("OK");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
await new Promise<void>(resolve => {
|
||||||
|
server.listen(0, resolve);
|
||||||
|
});
|
||||||
|
|
||||||
|
const port = (server.address() as any).port;
|
||||||
|
|
||||||
|
try {
|
||||||
|
const client = http2.connect(`http://localhost:${port}`);
|
||||||
|
|
||||||
|
const result = await new Promise<string>((resolve, reject) => {
|
||||||
|
client.on("error", reject);
|
||||||
|
|
||||||
|
const req = client.request({
|
||||||
|
":method": "POST",
|
||||||
|
":path": "/test",
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("response", headers => {
|
||||||
|
expect(headers[":status"]).toBe(200);
|
||||||
|
});
|
||||||
|
|
||||||
|
let data = "";
|
||||||
|
req.on("data", chunk => {
|
||||||
|
data += chunk;
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("end", () => {
|
||||||
|
resolve(data);
|
||||||
|
client.close();
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("error", reject);
|
||||||
|
|
||||||
|
// This is the pattern that was causing the bug:
|
||||||
|
// write() followed by end() should only send 2 DATA frames
|
||||||
|
const testData = Buffer.from("Hello, World!");
|
||||||
|
req.write(testData, "binary", () => {
|
||||||
|
req.end();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toBe("OK");
|
||||||
|
|
||||||
|
// We should receive exactly one data chunk (the test data),
|
||||||
|
// NOT two empty frames after it
|
||||||
|
expect(receivedDataFrames.length).toBe(1);
|
||||||
|
expect(receivedDataFrames[0].length).toBe(13); // "Hello, World!" is 13 bytes
|
||||||
|
} finally {
|
||||||
|
server.close();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test("http2 end() without data should send END_STREAM with no DATA frames", async () => {
|
||||||
|
const receivedDataFrames: Array<{ length: number }> = [];
|
||||||
|
|
||||||
|
const server = http2.createServer();
|
||||||
|
|
||||||
|
server.on("stream", (stream, headers) => {
|
||||||
|
stream.on("data", chunk => {
|
||||||
|
receivedDataFrames.push({ length: chunk.length });
|
||||||
|
});
|
||||||
|
|
||||||
|
stream.on("end", () => {
|
||||||
|
stream.respond({ ":status": 200 });
|
||||||
|
stream.end("OK");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
await new Promise<void>(resolve => {
|
||||||
|
server.listen(0, resolve);
|
||||||
|
});
|
||||||
|
|
||||||
|
const port = (server.address() as any).port;
|
||||||
|
|
||||||
|
try {
|
||||||
|
const client = http2.connect(`http://localhost:${port}`);
|
||||||
|
|
||||||
|
const result = await new Promise<string>((resolve, reject) => {
|
||||||
|
client.on("error", reject);
|
||||||
|
|
||||||
|
const req = client.request({
|
||||||
|
":method": "POST",
|
||||||
|
":path": "/test",
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("response", headers => {
|
||||||
|
expect(headers[":status"]).toBe(200);
|
||||||
|
});
|
||||||
|
|
||||||
|
let data = "";
|
||||||
|
req.on("data", chunk => {
|
||||||
|
data += chunk;
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("end", () => {
|
||||||
|
resolve(data);
|
||||||
|
client.close();
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("error", reject);
|
||||||
|
|
||||||
|
// Just call end() without any data
|
||||||
|
req.end();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toBe("OK");
|
||||||
|
|
||||||
|
// Should receive no data frames (just empty END_STREAM handled by _final)
|
||||||
|
expect(receivedDataFrames.length).toBe(0);
|
||||||
|
} finally {
|
||||||
|
server.close();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test("http2 end(data) should send data with END_STREAM in one frame", async () => {
|
||||||
|
const receivedDataFrames: Array<{ length: number }> = [];
|
||||||
|
|
||||||
|
const server = http2.createServer();
|
||||||
|
|
||||||
|
server.on("stream", (stream, headers) => {
|
||||||
|
stream.on("data", chunk => {
|
||||||
|
receivedDataFrames.push({ length: chunk.length });
|
||||||
|
});
|
||||||
|
|
||||||
|
stream.on("end", () => {
|
||||||
|
stream.respond({ ":status": 200 });
|
||||||
|
stream.end("OK");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
await new Promise<void>(resolve => {
|
||||||
|
server.listen(0, resolve);
|
||||||
|
});
|
||||||
|
|
||||||
|
const port = (server.address() as any).port;
|
||||||
|
|
||||||
|
try {
|
||||||
|
const client = http2.connect(`http://localhost:${port}`);
|
||||||
|
|
||||||
|
const result = await new Promise<string>((resolve, reject) => {
|
||||||
|
client.on("error", reject);
|
||||||
|
|
||||||
|
const req = client.request({
|
||||||
|
":method": "POST",
|
||||||
|
":path": "/test",
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("response", headers => {
|
||||||
|
expect(headers[":status"]).toBe(200);
|
||||||
|
});
|
||||||
|
|
||||||
|
let data = "";
|
||||||
|
req.on("data", chunk => {
|
||||||
|
data += chunk;
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("end", () => {
|
||||||
|
resolve(data);
|
||||||
|
client.close();
|
||||||
|
});
|
||||||
|
|
||||||
|
req.on("error", reject);
|
||||||
|
|
||||||
|
// end(data) should send data with END_STREAM
|
||||||
|
req.end(Buffer.from("Hello!"));
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(result).toBe("OK");
|
||||||
|
|
||||||
|
// Should receive exactly one data frame
|
||||||
|
expect(receivedDataFrames.length).toBe(1);
|
||||||
|
expect(receivedDataFrames[0].length).toBe(6); // "Hello!" is 6 bytes
|
||||||
|
} finally {
|
||||||
|
server.close();
|
||||||
|
}
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user