Conversation
|
Updated 5:56 PM PT - Sep 19th, 2026
✅ @robobun, your commit 12a30c6fe3c898836d834bd29f103d7dd5ae5656 passed in 🧪 To try this PR locally: bunx bun-pr 43566That installs a local version of the PR into your bun-43566 --bun |
|
Status: ready for review. Stacked on #43424, which is stacked on #43346. Reproduction ( import http2 from "node:http2";
const srv = http2.createServer();
const seen = [];
srv.on("stream", st => {
st.on("error", e => seen.push("server stream error " + e.code));
st.on("close", () => seen.push("server stream close"));
st.respond({ ":status": 200 });
st.end("first");
let sync = true;
st.end("second", err => seen.push("second end cb " + (err ? err.code : "ok") + (sync ? " (sync)" : " (async)")));
sync = false;
});
await new Promise(r => srv.listen(0, "127.0.0.1", r));
const c = http2.connect("http://127.0.0.1:" + srv.address().port);
await new Promise(resolve => {
const r = c.request({ ":path": "/" });
let body = "";
r.on("data", d => (body += d));
r.on("close", () => { seen.push("client close body=" + JSON.stringify(body) + " rstCode=" + r.rstCode); resolve(); });
r.end();
});
await new Promise(r => setImmediate(r));
console.log(seen.join("\n"));
c.close();
srv.close();The new tests are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Http2Stream#end() kept its own EndedCalled flag and returned early on a repeat call. It dropped the chunk, called the callback synchronously with no error, and skipped the argument validation. When end(chunk) threw, the flag stayed set and no later end() could end the stream. end() now delegates a repeat call to Writable#end, which reports the chunk as ERR_STREAM_WRITE_AFTER_END, defers the callback, and tracks the ending state itself. Only an end() nested inside another end() still returns early: respond() makes that call when the final _write sends the implicit response to a HEAD request.
…end() with a write in flight
bf76b74 to
038f89c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new commits address two of the earlier threads: onStreamWriteDone now sends END_STREAM for the write(a); end(); end(b) shape, and kEndingWithChunk/kInsideEnd are declared as class fields. Beyond the inline findings, I also checked the bare catch {} in onStreamWriteDone — with its fixed ("", "ascii", true) arguments the only throw path left in native writeStream is the dropped-stream-id one it is meant to absorb — and the per-chunk onStreamWriteDone.bind in _write/_writev, which is one small closure per non-final chunk next to a native write and not worth a separate comment.
Extended reasoning...
Findings are being posted inline, so this body only records what else was examined on this pushed version. The source diff was re-read against the prior review: the missing END_STREAM after a late end(chunk) and the undeclared symbol fields are now handled; the HEAD-request uncaught error (Node parity) and the cork/write/uncork/end(cb) HEAD shape remain as the PR describes. The bare catch was checked against h2_frame_parser.rs writeStream: with a numeric id, empty string payload, literal "ascii" encoding and boolean close, the only remaining throw is the "Invalid stream id" lookup, so the catch cannot hide a different failure. The bind allocation is a real but minor cost on a path already dominated by the native write and callback deferral.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (2):
- 🔴
src/js/node/http2.ts:2850—Existing Bun http2 servers using the canonical stream.respond(headers); stream.end(body) handler now crash with an unca… - Also unresolved: 1 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— pre-existing: a client or server whose peer sends RST_STREAM while a write is in flight can crash with an uncaught plainError: Invalid stream id, or get that error instead of ERR_HTTP2_STREAM_ERROR. The new onStreamWriteDone wraps its native call in try/catch for exactly this window, but the sibling sites do not. _final at src/js/node/http2.ts:2775 and the dispatch in _write/_writev call native.writeStream on an id the engine already evicted, and write_stream throws for an unknown id. Fix: handle the evicted-id case once for all callers, e.g. have write_stream (h2_frame_parser.rs:5881) dispatch the callback and return false like rst_stream does for unknown ids, which covers the 3 sites and lets the try/catch at :2086 go. …Extended reasoning...
…Same pattern at 3 sites (http2.ts:2775, http2.ts:2943, http2.ts:2901).
The window is the one the PR's own test builds: a peer sends HEADERS then RST_STREAM followed by enough bytes that the TLS socket delivers two reads in one turn. The first read processes the RST: Stream::free_resources (h2_frame_parser.rs:1932) pushes the id to pending_engine_stream_closes and JS is only told on nextTick (emitStreamErrorNT). The second read drains that list at dispatch depth 0 and calls this.streams.remove(&id) (h2_frame_parser.rs:3615-3618). After that write_stream (h2_frame_parser.rs:5881-5883) throws
Invalid stream idfor that id, unlike rst_stream (:5052-5075) which tolerates it. The write callback of the in-flight chunk was deferred by kDeferWriteCallback (nextTick or setImmediate) and runs after the turn, before emitStreamErrorNT destroys the stream. Take the test fixture minus the late write: req.write("body"); req.end() in 'response'. The deferred callback runs onwrite -> afterWrite -> finishMaybe -> prefinish -> _final. _final reaches native.writeStream(this.#id, "", "ascii",…Verification: pre-existing (the base branch has the identical
_final/_write/_writevcode; the diff adds the try/catch only inside the newonStreamWriteDonehelper while leaving its siblings unguarded). Trigger: an Http2 client whose transport is a JS-fed TLS socket (tls.connect({ socket: <Duplex> }), i.e. proxy tunnels / upgradeDuplexToTLS — exactly the transport the PR's own test builds) has a DATA…
…r turn respond() ends the writable side of a HEAD response. When the first write sends the implicit response, that end() runs inside the write dispatch, and the native layer completed the write synchronously. Writable then never finished the stream for a corked write followed by end(). The removed EndedCalled flag hid this for a corked end(chunk) only. The write now completes on a later turn, like every other write, so end() needs no early return for a nested call. Tests cover both corked forms and the remaining end() after end() forms.
|
Pushed c289010 and 12a30c6 for the review above.
|
This branch has not been deployed
1 participant
Stacked on #43424 (itself on #43346). The base is the #43424 branch. Only the last four commits are new.
Problem
Http2Stream#end(chunk, cb)on a stream whose writable side already ended drops the chunk and callscbsynchronously with no error. Node v26.3.0 emits'error'(ERR_STREAM_WRITE_AFTER_END) and passes it tocbon a later tick.respond()(204, 304, HEAD,endStream) and a body-lessrequest()end the writable side themselves, so their firstend(chunk)hits this.end()(src/js/node/http2.ts:2794) returns early on a privateEndedCalledflag and never reachesWritable#end. Anend(123)that throws leaves the flag set, so no laterend()can end the stream.Fix
end()goes toWritable#end, which rejects the chunk, validates the arguments, defers the callback and never runs_finalagain.Writablenever finished a corked stream. That write now completes on a later turn.end(chunk)emits'error'. With no'error'listener,stream.respond(h); stream.end(body)ends the process on any HEAD request, as in node and asstream.write(body)already does on main.test/js/node/http2/node-http2.test.js(19 new tests, 18 fail without the fix). Alsotest/js/node/http2/, vendoredtest-http2-*, grpc-js.Background
Writable#end(chunk)passes the chunk towrite(), which fails withERR_STREAM_WRITE_AFTER_ENDonce the stream is ending.Http2StreamhasautoDestroyoff, like node, so the error does not destroy it. node:http2: emit 'close' on a stream errored by a write() after end() #43346 makes that stream emit'close', and node:http2: send END_STREAM after a late write(), handle a peer reset at once like node #43424 makes it send END_STREAM when a write was in flight.respond()sends:status 200itself (the implicit response).Notes
Repro (
st.respond({ ":status": 200 }); st.end("first"); st.end("second", cb)in a'stream'handler):Decision for the reviewer. The behaviour change above is Node parity, and a remote peer can trigger it: one HEAD request ends a process whose
'stream'handler isstream.respond(h); stream.end(body)with no'error'listener on the stream. Node v26.3.0 does exactly that, and node's owntest-http2-head-request.jspins the same error forstream.write(), which Bun already emits on main. If Bun should stay lenient whenrespond()orrequest()ended the writable side (and not user code), say so. The change is then to pass the error to the callback only in that case. It needs a flag for who ended the stream, andend(chunk)would then differ fromwrite(chunk)on the same stream.Shapes compared with node v26.3.0. Each one gives the node result on this branch.
end("first"); end("second", cb)cb()sync'error',cb(err)async,'close'respond()for 204,endStream: trueor HEAD, thenend("body", cb)cb()sync'error',cb(err)async,'close'end("first"); end("", cb)cb()sync'error',cb(err)async,'close'end("first"), thenend("x", cb)in'finish'cb()sync'error',cb(err)async,'close'write("a"); end(); end("b", cb)(write in flight)cb()sync'error',cb(err)async,'close', the client receives"a"and the enddestroy(); end("x", cb), orend("x", cb)in'close'cb()synccb(ERR_STREAM_WRITE_AFTER_END)async, no'error'respondWithFD(fd); end("x", cb)cb()sync'error',cb(err)async,'close', file delivered in fullrequest().end("body", cb)orend("", cb)cb()sync'error',cb(err)async,'close', the server receives no bodyend("a"); end("b", cb), also on a pending streamcb()sync'error',cb(err)async,'close', the server receives"a"end(123)ERR_INVALID_ARG_TYPEend(123)throws, thenend("ok")"ok"end(cb)orend(null, cb)on an ended streamcb()synccb()async, no errorThe same results hold over a
duplexPairtransport. WithmaxConcurrentStreams: 1, three GET requests that each callend("body")all complete, so the errored stream frees its slot. The compat layer (Http2ServerResponse#end) does not pass a chunk tostream.end(), sores.end("a"); res.end("b", cb)is unchanged.Who sees the new
'error'.stream.respond(headers); stream.end(body)on a HEAD request, as described above.req.close(CANCEL),req.destroy()andsession.destroy()on the client, thenstream.end("late", cb)on the server one tick, one microtask, onesetImmediateand 50 ms later, with no'error'listener. The stream is destroyed by then, so the error goes to the callback only. No run raised an uncaught exception.The implicit response to a HEAD request.
EndedCalledalso made anend()insideend()a no-op, and one path relied on that. Take a HEAD request with norespond()call. The first_writesends the implicit response.respond()sees the HEAD request, puts END_STREAM on the HEADERS frame and callsthis.end(), inside the write dispatch. The native layer completed a write to that half-closed stream synchronously. With a corked write, thefinishMaybeinonwritestill seeskBuffered, and nothing calls it again.ServerHttp2Stream#_writeand_writevnow drop the body of that response themselves and complete the write on a later turn, soWritablefinishes the stream.respond()end("body", cb), corked or notcb(),'finish','close''error',cb(err)cork(); write("a"); end(cb)cb(),'finish','close''error',cb(err)cork(); write("a"); uncork(); end(cb)cb()sync, no'finish', no'close'from the request's'end'handlercb(),'finish','close''error',cb(err)write("a"); end("b", cb)cb(),'finish','close''error',cb(err),'close''error',cb(err)Node ends the writable side of a HEAD stream when it creates the stream, so every chunk fails there. Bun cannot do that until an
end()beforerespond()puts END_STREAM on the HEADERS frame (#38170). An earlier version of this PR kept an early return inend()for the nested call. The review asked for a fix at the source, and this is it.Not in this PR.
respond(), the firstwrite()orend(chunk)drops its chunk without an error. Node fails it. See the paragraph above.'finish'before the'error'. Node emits no'finish'there. Also on main.'close'only when the session closes (node:http2: release server push streams once their response ends #38082). Also on main._final,_writeand_writevcallnative.writeStreamwith the id of a stream that a peer RST_STREAM already made the native layer drop.writeStreamthrowsInvalid stream idfor it. node:http2: send END_STREAM after a late write(), handle a peer reset at once like node #43424 guards only its own new call. A fix inwrite_stream(src/runtime/api/bun/h2_frame_parser.rs), which would call back and return likerst_streamdoes for an unknown id, covers all three sites.The flag.
EndedCalledonce told_writethatend()ran, to put END_STREAM on the last DATA frame._writenow reads_writableState.ending, so only the early return still used the flag.Why the stack. The late
end(chunk)errors the Duplex without destroying it, the same state a latewrite()produces. On main such a stream never emits'close'(#43346 fixes that). When a write is in flight at the firstend(), the END_STREAM has to come from_final, andWritablenever calls_finalon an errored stream (#43424 fixes that). With this PR on #43346 alone,write("a"); end(); end("b")never ends the response. On #43424 it does, and two of the new tests cover it.Overlap with #33489. That PR changes the same early return to
super.end(undefined, undefined, callback), which defers the callback and keeps the silent drop of the chunk. This PR removes that block, so it contains that source change. The 15 tests of #33489 pass on this branch. The PR that merges second needs a rebase ofend().Tests. 19 new tests in the
end() after end()block. 18 fail on the base without thesrc/change. The one that passes there is the corkedend(chunk)on a HEAD request, which the removed flag covered. It fails when the asynchronous completion is removed. 17 of the 19 also pass under node v26.3.0 (run with a smalldescribe/it/expectshim). The other two pin the Bun-only implicit response to a HEAD request.Suites on the debug+ASAN build.
test/js/node/http2/: 611 pass, 6 skip, 0 fail. All 256 vendoredtest-http2-*files and the 22test-diagnostics-channel-http2-*,test-stream-pipeline-http2andtest-worker*-http2-*files pass. grpc-js: 322 pass, 6 fail, the same failures as without this change (5 need public DNS,test-tonic). The http2 regression tests,serve-http2*.test.tsandfetch-http2-client.test.ts: 425 pass, 0 fail.Self-review. 7 concerns raised, 7 addressed: the nested
end()above (found by the review, fixed and tested), and six about the tests (argument validation on an ended stream not pinned, no test after destroy, no check of what reached the wire, an unhandled rejection on the failure path of one test, two unclear comments, test names).[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file