Conversation
WalkthroughThis PR modifies ChangesHTTP/2 endStream fix
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClientHttp2Session
participant Http2Stream
Caller->>ClientHttp2Session: request(options)
ClientHttp2Session->>ClientHttp2Session: compute endStream (respect explicit false)
alt endStream true
ClientHttp2Session->>Http2Stream: req.end()
end
ClientHttp2Session-->>Caller: return req
Estimated code review effort: Medium Related issues: None specified. Related PRs: None specified. Suggested labels: node.js, http2, bug Suggested reviewers: None specified.
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
Updated 8:02 AM PT - Jul 29th, 2026
❌ @robobun, your commit 860820a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33489That installs a local version of the PR into your bun-33489 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues, but this threads req.end() through six distinct return paths in request() with ordering that depends on native stream state and _final timing — worth a human sanity check on the placement relative to #parser.request() and cork().
Extended reasoning...
Overview
This PR fixes two Node.js compat divergences in ClientHttp2Session#request() (src/js/node/http2.ts): (1) requests created with endStream: true (explicit or the GET/HEAD/DELETE default) spuriously emit 'aborted' because the JS Duplex's writable side was never closed, and (2) an explicit endStream: false on those methods was being overridden back to true. The fix defaults endStream only when unset, computes the effective value once, and calls req.end() on every return path where it's true — mirroring Node's if (options.endStream) stream.end(). It also makes Http2Stream#end() return this on its idempotent short-circuit, and gates the preemptive content-length rejection on the effective endStream rather than the method alone. Eight new tests cover the variant matrix.
Security risks
None. This is stream lifecycle bookkeeping in the Node compat layer; no auth, crypto, input parsing, or trust boundary is touched.
Level of scrutiny
Medium-high. The change itself is small and well-motivated (with cited Node source), but it inserts req.end() at six different points in request() — before/after native #parser.request(), before cork(), on error paths, and on the queued-behind-concurrency-limit path — and correctness depends on subtle interactions: _final parking on 'ready' for pending streams, the native side suppressing an empty DATA frame because can_send_data() is false in HALF_CLOSED_LOCAL, and _writableState.ending being set synchronously so _destroy/close no longer take the aborted branch. This is the kind of ordering that's easy to get wrong in one path and only show up under load or with specific peers.
Other factors
The PR description is thorough, the author ran the full test/js/node/http2/ suite and all 272 test-http2-* node parallel tests with no new failures, and the bug hunter found nothing. The tests look solid (they assert exact event sequences, writableEnded at request time, and body echo). I'm deferring only because the http2 client request path is load-bearing for grpc-js/got/http2-wrapper and the placement of end() relative to native registration and corking is subtle enough that a maintainer familiar with the native side should confirm the HALF_CLOSED_LOCAL suppression claim.
|
Fair concern: the The suppression claim, verified on the wireA raw TCP server that decodes every frame the client sends, against node v26.3.0 and bun (debug) before and after the change: Two things fall out of that:
This is now the test The queued pathThe path where On the placementTwo constraints, and together they're why this isn't a single
The other four sites are streams that never reach the wire (bad All 10 tests in the |
There was a problem hiding this comment.
I didn't find any bugs — the fix is well-reasoned and the wire-level frame test is a nice proof — but this threads req.end() into six different return paths of the client request() hot path with ordering that depends on native-side state (_final → writeStream suppressed by HALF_CLOSED_LOCAL), so it's worth a human familiar with the http2 native layer confirming those assumptions.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug in src/js/node/http2.ts where every body-less HTTP/2 client request (explicit { endStream: true } or implicit via GET/HEAD/DELETE) spuriously emitted 'aborted' and set stream.aborted = true on normal completion. The root cause is that ClientHttp2Session#request() never closed the JS Duplex's writable half after the native layer sent END_STREAM on the HEADERS frame, so _destroy() misread the still-open writable side as "peer cut us off."
The fix has three parts, all in request():
- Default
endStreamfrom the method only when the caller left it unset (an explicitendStream: falseon GET/HEAD/DELETE now reaches native and can carry a body). - Call
req.end()on every return path (6 sites: invalid-path reject, content-length reject, pre-aborted signal, queued-behind-concurrency-limit, out-of-streams, and the main submit path) when the effectiveendStreamis true. - Gate the preemptive content-length rejection on the effective
endStreamrather than the method alone.
Plus a one-line fix: Http2Stream#end() now returns this on its idempotent short-circuit (was undefined), matching the Writable#end() contract.
~200 lines of new tests cover: no-aborted for all four endStream shapes, explicit endStream: false delivering a body on GET/DELETE, close() on body-less requests, end() chainability, the queued-behind-maxConcurrentStreams path, and a raw-socket frame decoder proving no extra DATA frame goes on the wire.
Security risks
None. This is client-side stream state and event emission for Node.js compat. No auth, crypto, input parsing, or trust-boundary changes.
Level of scrutiny
Medium-high. This is not a mechanical change:
- Every HTTP/2 client request flows through
request(); the change fans out across 6 return paths, each with its own ordering constraint (e.g., on the submit pathreq.end()must follow the native#parser.request()so the stream is registered before_finalruns; on the queued path it precedescork()and_finalparks on'ready'). - Correctness depends on a native-side invariant the diff can't show: that
writeStream(..., close=true)is suppressed when the stream is alreadyHALF_CLOSED_LOCAL, soend()doesn't emit a stray empty DATA frame. The wire-level test verifies this empirically, which is reassuring, but a reviewer familiar withsrc/runtime/node/h2_frame_parser.rs(or equivalent) should confirm the assumption holds generally. - There's a behavioral change beyond the aborted fix:
endStream: falseon GET/HEAD/DELETE now sends a body where it was previously silently dropped. This matches Node but is a user-visible behavior change.
Other factors
- The PR description is exceptionally thorough: root cause traced to Node's own
lib/internal/http2/core.js, before/after verification, full http2 suite run (377 tests, same 2 pre-existing failures), and all 272test-http2-*node parallel tests with identical pass/fail sets. - The wire-level test that decodes raw frames from a
net.Serverdirectly addresses the main risk (extra DATA frame). - No CODEOWNERS entry for this path.
- No bugs found by the automated review.
- CI is still building at time of review.
Given the breadth of code paths touched in a hot path and the reliance on native-side state machine behavior, I'm deferring rather than auto-approving.
The wire test shows it empirically; here is the static version, so a reviewer doesn't have to take the test's word for it. Nothing in this PR touches
So On the intentional behaviour changeRight that |
c65ac4c to
8e58cf9
Compare
|
Rebased onto
Earlier comments in this thread reference |
8e58cf9 to
5d5020d
Compare
There was a problem hiding this comment.
Thanks for addressing the synchronous-callback issue — forwarding to super.end() is a cleaner fix than what I suggested, and the two new tests pin it. I didn't find anything further, but the coupled changes to the client request() path (six req.end() sites, the endStream defaulting, and the content-length gate) interact closely enough with the native stream state machine that someone familiar with h2_frame_parser.rs should give this a look.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility bug in src/js/node/http2.ts: client requests created with endStream: true (explicit or the GET/HEAD/DELETE default) were emitting a spurious 'aborted' event on normal completion because the JS writable half was never closed. It also fixes a secondary divergence where an explicit endStream: false on those methods was overridden back to true, silently dropping the body. The fix has three coupled parts in ClientHttp2Session#request(): (1) only default endStream from the method when the caller left it unset, (2) call req.end() at six sites when the effective endStream is true so _writableState.ending is set before _destroy/close inspect it, and (3) gate the preemptive content-length rejection on the effective endStream rather than the method. A fourth change rewrites Http2Stream#end()'s already-ended short-circuit to delegate to super.end() — this addresses the synchronous-callback issue I flagged in a prior review, which the author confirmed and fixed. A new 241-line test file with 11 tests covers all variants including a raw-TCP wire-level frame decoder and a maxConcurrentStreams queued-path test.
Security risks
None identified. This is stream-lifecycle bookkeeping in the Node compat layer; no auth, crypto, permissions, or untrusted-input parsing is touched. The endStream: false behaviour change only affects callers who explicitly opted into sending a body on GET/HEAD/DELETE (matching Node), and the previous behaviour was silent data loss rather than a security boundary.
Level of scrutiny
This warrants human review. The HTTP/2 client request path is production-critical (used by grpc-js, got, http2-wrapper per the description), and the correctness of the fix rests on a specific interaction with the native layer: req.end() after an endStream HEADERS frame must be a no-op on the wire because can_send_data() returns false for HALF_CLOSED_LOCAL. The author has verified this both empirically (wire-level frame decoder test) and by static trace through h2_frame_parser.rs, and the reasoning looks sound to me — but the six req.end() insertions are placed at specific points relative to #parser.request(), cork(), and error-path nextTicks for stated ordering reasons, and a maintainer who owns that native code should confirm the placement and the state-machine assumptions hold generally (including under waitForTrailers and abort-signal races).
Other factors
My earlier inline comment about the synchronous end(cb) callback was addressed (the fix delegates to Writable#end, which is stronger than my suggestion — it also restores ERR_STREAM_ALREADY_FINISHED/ERR_STREAM_DESTROYED reporting and the return value), and two tests now pin that behaviour. The bug-hunting pass on the current revision found nothing. Test coverage is thorough (11 tests, all fail on main, wire-level and queued-path variants included), and the author reports no regressions across 272 test-http2-* Node parallel tests and test/js/node/http2/. The change is well-reasoned and well-verified — I'm deferring purely because it's a multi-part behavioural change on a hot Node-compat path, not because I see a problem.
04ee69b to
80df27b
Compare
|
You named two scenarios worth checking, Why the abort case was worth worrying aboutThe native aborted(self, stream, error, old_state) {
stream.rstCode = constants.NGHTTP2_CANCEL;
// if writable and not closed emit aborted
if (old_state != 5 && old_state != 7) { // 5 = HALF_CLOSED_LOCAL
stream[kAborted] = true;
stream.emit("aborted");
}Every Answer: no, and node agrees, because node skips the same branch for the same reason. The PR flips exactly the two rows where What I addedMy tests only asserted
The first pair passes on waitForTrailersRe-checked with the Both match.
|
Status: ready to merge, CI green
What is verified
|
80df27b to
aac9304
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
aac9304 to
acd46ff
Compare
Since request() closes the writable side of every body-less request itself (setupRequestEndAndSignal, added in #32488), a user's first .end(cb) lands on the already-ended short-circuit, which invoked the callback synchronously and returned. Writable#end never does: the callback goes through the kOnFinished queue or a deferred tick with ERR_STREAM_ALREADY_FINISHED/ERR_STREAM_DESTROYED. Forward to super.end(undefined, undefined, callback) instead. kEnding is already set, so super.end() skips finishMaybe and cannot re-enter _final; it only routes the callback.
The two end() callback cases fail on main and pass with the fix: - end() on an already-ended request stays chainable and defers the callback - end(callback) once the writable side finished reports ERR_STREAM_ALREADY_FINISHED The rest pass on main (via #32488) and pin that contract: no spurious 'aborted' on the four endStream shapes, endStream: false delivering a body, close() on a body-less request, the queued-behind-maxConcurrentStreams path, the wire shape (no DATA frame on a stream the HEADERS frame already half-closed), and the negative (an AbortController firing mid-body and a peer RST_STREAM(CANCEL) still emit 'aborted' on a request whose writable half is open, and stay quiet on a body-less one).
acd46ff to
860820a
Compare
robobun
changed the title
RetargetedThe spurious- What #32488 did not cover is the So the PR now carries only that: the |
There was a problem hiding this comment.
LGTM — the retargeted diff is a focused 3-line delegation to Writable#end, which is the layer that owns this contract.
What was reviewed:
- Traced
super.end(undefined, undefined, cb)throughwritable.ts:770-826: withkEndingset it skipsfinishMaybe(no_finalre-entry), routes cb vianextTick/kOnFinished, reportsERR_STREAM_ALREADY_FINISHED/ERR_STREAM_DESTROYED, and returnsthis. - Checked the dropped-chunk and non-function-callback cases match old behavior; the uncork side effect is idempotent (first
end()already uncorked). - Test file: no timers, awaits events,
listen(0), error handlers wired to reject, cleanup infinally,./helpersexports the frame utilities used.
Extended reasoning...
Overview
After retargeting, the source diff is 3 lines in src/js/node/http2.ts: the EndedCalled short-circuit in Http2Stream#end() now calls return super.end(undefined, undefined, callback) instead of invoking callback() synchronously and returning this. The 316-line test file covers the end(cb) timing/error contract plus 13 tests pinning the endStream behavior that #32488 already fixed on main.
Security risks
None. This is Node-compat callback ordering in a builtin JS module; no untrusted-input parsing, no auth/crypto/permissions, no native memory.
Level of scrutiny
Low-to-medium. The src change replaces hand-rolled behavior with delegation to the parent class that owns the contract — the "fix at the layer that owns the invariant" pattern the review guide asks for. I verified against src/js/internal/streams/writable.ts:799-826 that with kEnding set (guaranteed, since the first end() set it before setting EndedCalled), Writable#end skips finishMaybe entirely and only routes the callback. Edge cases: chunk is passed as undefined so a repeat end(chunk) stays a silent drop (pre-existing, called out in the description); a non-function callback is a no-op in both old and new code; uncork is idempotent.
Other factors
My prior inline finding on this PR (the synchronous callback) is exactly what the retargeted PR now fixes, and the thread is resolved. The comment-cop note about the long comment was addressed (tightened to one line naming the non-obvious invariant). Two of the 15 tests fail on main and pass with the fix; the other 13 pin #32488's contract. Tests follow harness conventions: dynamic ports, event-driven awaits (no sleep), error handlers reject the awaited promise, resources released in finally, it.each for the variant matrix. The wire-level test reuses the existing test/js/node/http2/helpers.js frame utilities. CI's only red was napi.test.ts on Windows, unrelated to node:http2.
|
Tests added or changed in this PR must pass on both Bun and Node.js v26.3.0, so they only assert behavior Node actually has. |
|
@robobun please run the tests this PR adds or changes on Node.js v26.3.0 and report whether each one passes or fails. |
|
@cirospaciari ran the one test file this PR adds, unmodified, under Node.js v26.3.0 and under
Three consecutive Node runs: How it was runThe file imports This was the expectation going in: every assertion in the file was written against Node's observed behaviour first (the probes are in the thread above), including the exact event orderings, the The shim, for reproducibility (3 files, ~60 lines)
import { register } from "node:module";
import { pathToFileURL } from "node:url";
register(pathToFileURL("/tmp/buntest-shim/hooks.mjs").href);
export async function resolve(specifier, context, nextResolve) {
if (specifier === "bun:test") {
return { url: "file:///tmp/buntest-shim/bun-test.mjs", shortCircuit: true };
}
try {
return await nextResolve(specifier, context);
} catch (err) {
// bun resolves extensionless relative imports; node's ESM resolver does not.
if (err?.code === "ERR_MODULE_NOT_FOUND" && specifier.startsWith(".")) {
for (const ext of [".js", ".ts", ".mjs", ".cjs"]) {
try { return await nextResolve(specifier + ext, context); } catch {}
}
}
throw err;
}
}
import { describe as nDescribe, it as nIt } from "node:test";
import assert from "node:assert/strict";
import { format } from "node:util";
export const describe = (name, fn) => nDescribe(name, fn);
function makeIt(base) {
const it = (name, fn) => base(name, fn);
it.each = table => (nameFmt, fn) => {
for (const row of table) {
const args = Array.isArray(row) ? row : [row];
let i = 0;
const title = nameFmt.replace(/%[sdifjo%]/g, m => (m === "%%" ? "%" : format("%s", args[i++])));
base(title, () => fn(...args));
}
};
return it;
}
export const it = makeIt(nIt);
export const test = it;
function matchObject(actual, expected, path = "") {
for (const key of Object.keys(expected)) {
const e = expected[key], a = actual?.[key];
if (e !== null && typeof e === "object" && !Array.isArray(e) && !Buffer.isBuffer(e)) {
matchObject(a, e, `${path}.${key}`);
} else {
assert.deepStrictEqual(a, e, `toMatchObject mismatch at ${path}.${key}`);
}
}
}
export function expect(actual) {
return {
toBe: expected => assert.strictEqual(actual, expected),
toEqual: expected => assert.deepStrictEqual(actual, expected),
toMatchObject: expected => matchObject(actual, expected),
};
} |
## What does this PR do?
Fixes the event sequence emitted by `ClientHttp2Stream.close(code)` to
match Node.js:
| code | Node.js | Bun before | Bun after |
| --- | --- | --- | --- |
| `NGHTTP2_NO_ERROR` (0) | `end`, `close` | `end`, `close` | `end`,
`close` |
| `NGHTTP2_CANCEL` (8) | `end`, `close` | `end`, **`error`**, `close` |
`end`, `close` |
| any other (e.g. 2, 11) | `error`, `close` | **`end`**, `error`,
`close` | `error`, `close` |
### Reproduction
A raw h2c server sends `200` + one DATA frame and never sends
`END_STREAM`. The client calls `req.close(code)` from inside the first
`data` handler:
```js
st.on("data", () => { st.close(code); });
```
<details><summary>Full repro (verified against Node v26.3.0)</summary>
```
close( 0): DIFF got=[resp,data,aborted,end,close:0] node=[resp,data,end,close:0]
close( 8): DIFF got=[resp,data,aborted,end,err:ERR_HTTP2_STREAM_ERROR,close:8] node=[resp,data,end,close:8]
close( 2): DIFF got=[resp,data,aborted,end,err:ERR_HTTP2_STREAM_ERROR,close:2] node=[resp,data,err:...,close:2]
close(11): DIFF got=[resp,data,aborted,end,err:ERR_HTTP2_STREAM_ERROR,close:11] node=[resp,data,err:...,close:11]
```
(The extra `aborted` on every row is a separate issue, already tracked
in #33489.)
</details>
### Cause
1. `Http2Stream.prototype.close()` called `this.push(null)`
unconditionally before scheduling the RST, so `'end'` fired on the next
tick for every code, before `destroy()` could set the errored state. For
error codes that meant both `'end'` and `'error'` on a readable the
stream itself had just killed with `RST_STREAM`.
2. `emitStreamErrorNT` (the stream teardown path reached from
`native.rstStream(id, code)`) synthesized `ERR_HTTP2_STREAM_ERROR` for
any nonzero code, with no exemption for `NGHTTP2_CANCEL`. Node documents
CANCEL as the silent "abort this fetch" idiom: `_destroy` already
exempts it, but this path bypassed that.
### Fix
- In `close()`, only `push(null)` when `this.pending || code ===
NGHTTP2_NO_ERROR || code === NGHTTP2_CANCEL`, matching Node's
`finishCloseStream`: a pending stream (no id yet) ends its readable
cleanly regardless of code, and a non-pending stream closed with an
error code lets `_destroy` end the readable so `'end'` is suppressed
once the error is set.
- Drop the `push(null)` from `pushToStream`'s closed-stream branch: data
arriving after `close()` is discarded, and `_destroy` ends the readable
with the right event.
- Exempt `NGHTTP2_CANCEL` in `emitStreamErrorNT`, matching the exemption
`_destroy` already applies.
### Verification
New test matrix over `{0, 8, 2, 11}` in
`test/js/node/http2/node-http2-client-close.test.ts` asserts the exact
event sequence in three situations: `close(code)` after data has started
(raw h2c server that never sends END_STREAM), `close(code)` on a
non-pending stream before any response, and `close(code)` on a pending
stream (no id yet, `request()` before the session connects). The file
uses `node:test` + `node:assert` so it runs unchanged under `node
--test`, and it spawns Node on itself from `bun test`: all 12 cases pass
on Node v26.3.0 and on this branch, 7 of 12 fail on main. Full
`node-http2.test.js` suite (379 tests),
`test-http2-client-rststream-before-connect.js`, and the other
rst/cancel `test-http2-*` Node parallel tests pass.
<!-- robobun:evidence:begin -->
---
**[human-review]** gate passed · iteration 9 · 2 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: 7 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-client-close.test.ts"
bun test v1.4.3 (4ff9193)
test/js/node/http2/node-http2-client-close.test.ts:
(pass) ClientHttp2Stream.close(code) event sequence after data > close(0) [846.54ms]
107 | ] as const) {
108 | test(`close(${code})`, async () => {
109 | const srv = rawH2Server();
110 | const port = await listen(srv);
111 | try {
112 | assert.deepStrictEqual(await collectEvents(port, code), expected);
^
AssertionError: Expected values to be strictly deep-equal:
+ actual - expected
[
'response',
'data',
'end',
+ 'error:ERR_HTTP2_STREAM_ERROR',
'close:8'
]
generatedMessage: true,
actual: [ "response", "data", "end", "error:ERR_HTTP2_STREAM_ERROR", "close:8" ],
expected: [ "response",
"data", "end", "close:8"
],
operator: "deepStrictEqual",
diff: "simple",
code: "ERR_ASSERTION"
at /workspace/bun/test/js/node/http2/node-http2-client-close.test.ts:112:16
at node:test:1781:26
at executeTe
... (truncated)
release without fix: 7 FAILED
bun test v1.4.3-canary.1 (4ff9193)
test/js/node/http2/node-http2-client-close.test.ts:
(pass) ClientHttp2Stream.close(code) event sequence after data > close(0) [12.37ms]
107 | ] as const) {
108 | test(`close(${code})`, async () => {
109 | const srv = rawH2Server();
110 | const port = await listen(srv);
111 | try {
112 | assert.deepStrictEqual(await collectEvents(port, code), expected);
^
AssertionError: Expected values to be strictly deep-equal:
+ actual - expected
[
'response',
'data',
'end',
+ 'error:ERR_HTTP2_STREAM_ERROR',
'close:8'
]
generatedMessage: true,
actual: [ "response", "data", "end", "error:ERR_HTTP2_STREAM_ERROR", "close:8" ],
expected: [ "response",
"data", "end", "close:8"
],
operator: "deepStrictEqual",
diff: "simple",
code: "ERR_ASSERTION"
at /workspace/bun/test/js/node/http2/node-http2-client-close.test.ts:112:16
at node:test:1445:26
at executeTestNode (node:test:1448:63)
at processTicksAndRejections (native:7:39)
(fail) ClientHttp2Stream.close(code) event sequence after data > close(8) [4.85ms]
107 | ] as const
... (truncated)
```
</details>
<details><summary>passes on PR (with fix)</summary>
```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/node-http2-client-close.test.ts"
bun test v1.4.3 (4ff9193)
test/js/node/http2/node-http2-client-close.test.ts:
(pass) ClientHttp2Stream.close(code) event sequence after data > close(0) [669.27ms]
(pass) ClientHttp2Stream.close(code) event sequence after data > close(8) [116.17ms]
(pass) ClientHttp2Stream.close(code) event sequence after data > close(2) [73.51ms]
(pass) ClientHttp2Stream.close(code) event sequence after data > close(11) [80.02ms]
(pass) ClientHttp2Stream.close(code) before response > close(0) [178.78ms]
(pass) ClientHttp2Stream.close(code) before response > close(8) [76.89ms]
(pass) ClientHttp2Stream.close(code) before response > close(2) [61.56ms]
(pass) ClientHttp2Stream.close(code) before response > close(11) [56.12ms]
(pass) ClientHttp2Stream.close(code) while pending > close(0) [57.72ms]
(pass) ClientHttp2Stream.close(code) while pending > close(8) [50.58ms]
(pass) ClientHttp2Stream.close(code) while pending > close(2) [51.49ms]
(pass) ClientHttp2Stream.close(code) while pending > close(11) [4
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
target linux-x64-gnu
build type Release
build dir ./build/release
revision 830ec5f
features baseline
23 deps, 131 codegen, 1172 objects in 686ms
ninja: Entering directory `/workspace/bun/build/release'
[1/1244] install /workspace/bun
bun install v1.4.3-canary.1 (4ff9193)
Checked 22 installs across 61 packages (no changes) [13.00ms]
[2/1244] gen ErrorCode+*.h
[3/1244] install /workspace/bun/packages/bun-error
bun install v1.4.3-canary.1 (4ff9193)
Checked 1 install across 2 packages (no changes) [2.00ms]
[4/1244] gen bindgenv2
[5/1244] install /workspace/bun/src/node-fallbacks
bun install v1.4.3-canary.1 (4ff9193)
Checked 111 installs across 104 packages (no changes) [12.00ms]
[6/1244] gen node-fallbacks/react-refresh.js
Bundled 1 module in 5ms
react-refresh.js 4.81 KB (entry point)
[7/1244] fetch tinycc
[tinycc] up to date
[8/1243] gen .bind.ts → GeneratedBindings.cpp
[9/1243] fetch zlib
[zlib] up to date
[10/1243] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[11/1216] gen bake.{client,server,error}.js
-> bake.client.js, bake.serve
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
src/js/node/http2.ts | 16 +-
test/js/node/http2/node-http2-client-close.test.ts | 199 +++++++++++++++++++++
2 files changed, 205 insertions(+), 10 deletions(-)
```
</details>
**gate history** · 4 passed · 1 rejected · iteration 9
<details><summary>evidence per changed file</summary>
```
file reads edits tests
src/js/node/http2.ts 15 16 36
test/js/node/http2/node-http2-client-close.test.ts 4 8 33
```
</details>
<!-- robobun:evidence:end -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
|
#43566 changes the same block in The 15 tests in |
This branch has not been deployed
2 participants
Repro
Since #32488,
request()closes the writable side of every body-less request itself. A user's first.end(cb)on that stream now lands onHttp2Stream#end()'s already-ended short-circuit, which invokes the callback synchronously. Node'sWritable#endnever does.The same short-circuit also swallows the
ERR_STREAM_ALREADY_FINISHED/ERR_STREAM_DESTROYEDthatWritable#endreports to the callback once the stream is past those states.Cause
The already-ended path hand-rolled half the
Writable#endcontract (the return value) and dropped the other half:Before #32488 nothing inside
request()setEndedCalled, so a user's first.end(cb)fell through tosuper.end(cb)and stayed async. Now every GET/HEAD/DELETE, and every explicit{ endStream: true }, routes the first user.end(cb)into this branch.Fix
Forward to
Writable#end:kEndingis already set, sosuper.end()skips thefinishMaybebranch and cannot re-enter_final; all it does is route the callback, through thekOnFinishedqueue, withERR_STREAM_ALREADY_FINISHED/ERR_STREAM_DESTROYEDwhere node reports them, and it returns the stream. Passingundefinedfor the chunk keeps the existing silent-drop of a repeatend(chunk)(a separate pre-existing divergence).History
This PR originally fixed the spurious
'aborted'onendStreamrequests (see the thread). #32488 landed the same fix independently and now covers that whole area. Running this PR's 15-test file against currentmainshowed the headline bug is gone, but theend(cb)timing is not, for the same reason review caught it in the earlier version of this PR. So the PR now carries only that remaining piece.Verification
New tests in
test/js/node/http2/node-http2-end-stream.test.ts(15 tests). Two fail onmain:end()on an already-ended request stays chainable and defers the callbackend(callback)once the writable side finished reportsERR_STREAM_ALREADY_FINISHEDThe other 13 pass on
main(#32488 fixed them) and pin the contractmainnow claims: no'aborted'on the four endStream shapes, explicitendStream: falsedelivering a body on GET/DELETE,close()on a body-less request, the queued-behind-maxConcurrentStreamspath, the wire shape (no DATA frame on a stream the HEADERS frame already half-closed), and the negative contract (anAbortControllerfiring mid-body and a peerRST_STREAM(CANCEL)still emit'aborted'on a request whose writable half is open, and stay quiet on a body-less one).Before / after, and no regressions
Three consecutive runs of the new file: 15 pass each time.
bun bd test test/js/node/http2/: 423 pass / 1 fail, versus 421 pass / 3 fail onmainwith this test file in place. The one remaining failure is the pre-existingdoes not hold *Stream across user-controlled options gettersdebug+ASAN timeout.All
test-http2-*files undertest/js/node/test/parallel/against the debug build: 1 failure before and after, identical.[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 4
evidence per changed file