Repository navigation
Conversation
…constructor A `type: "direct"` stream does not run start(), so a source without pull() can never write or close. The constructor accepted it, and each consumer gave it a meaning of its own: readers rejected with a TypeError, and a native sink took it as a complete empty body and left the stream readable and unlocked, so a later read() never settled. The constructor now throws the TypeError that it already throws for a pull that is not a function. The two guards for a missing pull in readDirectStream and oneShotCallPull are unreachable and are deleted.
|
Updated 8:31 PM PT - Nov 10th, 2026
✅ @robobun, your commit 5a381f0169e5139dffed10c46033aa864d0b9685 passed in 🧪 To try this PR locally: bunx bun-pr 48522That installs a local version of the PR into your bun-48522 --bun |
|
Status Reproduced on release builds of 1.4.4 and of main (89452d2): const s = new ReadableStream({ type: "direct" });
await Bun.write("/tmp/out.txt", new Response(s)); // resolves 0
s.locked; // false
await s.getReader().read(); // never settlesWith this PR, the first line throws The 4 new cases in |
|
No actionable comments were generated in the recent review. WalkthroughDirect-stream construction now throws a ChangesDirect stream pull requirement
Possibly related PRs
Suggested reviewers: Merge Risk:
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I checked the deleted null-pull guards: the only other creator of JSDirectStreamSource (src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp:546) passes the runtime's lazily-initialized asyncIterableSourcePull JSFunction, which is never null, and callDirectPull keeps its own null-source guard, so no remaining path reaches pullFunction() with an empty field. The dictionary conversion leaves dict.pull empty exactly when absent or undefined, so the new check and the four test variants line up. I did not run the test file; by reading, the base constructor passed nullptr and succeeded, so the new cases fail there. The construction-time versus first-use choice the description asks about is a maintainer call, and packages/bun-types/bun.d.ts has a CODEOWNER.
Extended reasoning...
The change makes the ReadableStream constructor reject a type: "direct" source without a callable pull, deletes the two now-unreachable null-pull guards in BunStreamConsumers.cpp and BunStreamSource.cpp, adds an ASSERT in JSDirectStreamSource::finishCreation, and updates the header comment, docs, .d.ts JSDoc, and a four-case it.each test asserting exact error name and message. It touches no security-sensitive surface. Deferring rather than approving because it is a user-visible tightening of a Bun extension that the author explicitly flags as needing a maintainer decision, and because the .d.ts file is CODEOWNER-owned.
A direct source without pull() gets its own TypeError text. The text names the condition (type is "direct") and the reason (a direct stream does not call start()). A pull that is present and not a function keeps the existing text.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the two deleted guards are genuinely unreachable: both JSDirectStreamSource::create callers now pass a non-null pull (the constructor after its new check, and BunAsyncIterableSource.cpp:546 with a runtime host function), the pull local in readDirectStream is still consumed at BunStreamSource.cpp:754, and the remaining null-check at JSDirectStreamController.cpp:720 guards a null source rather than a null pull. pull: null still routes through the existing "must be a function" error at JSReadableStream.cpp:179. I did not run the test file or BUN_JSC_validateExceptionChecks=1; this is from reading only.
Extended reasoning...
The change adds a construction-time TypeError for a type: "direct" source without pull, removes two downstream null guards, and updates the header comment, docs, .d.ts JSDoc and tests; it touches no security-sensitive surface. The .d.ts file is CODEOWNER-owned and the PR explicitly asks a maintainer to decide between rejecting at construction versus at first use, so a human decision is required regardless of the code being internally consistent.
Problem
new ReadableStream({ type: "direct" })accepts a source withoutpull. A direct stream never runsstart(), so that source can never write.Bun.write,Bun.serve,fetchupload,Bun.spawnstdin,HTMLRewriter, S3) takes it as an empty body and leaves the stream readable and unlocked, sogetReader().read()never settles afterwards.pull(src/jsc/bindings/webcore/streams/JSReadableStream.cpp:344). Nothing else lets one in.Fix
TypeError: The underlying source's 'pull' property is required when 'type' is "direct": a direct stream does not call start()for a direct source withoutpull.pullinreadDirectStreamandoneShotCallPullare dead and deleted.test/js/web/streams/streams.test.js(4 new cases fail on main). Self-reviewed: 25 concerns raised, 19 addressed (Notes).Background
type: "direct"stream givespull()a controller that writes to the destination. A native sink callspull()once.Downsides
pullnow throws at construction, even if the stream is only cancelled (thecancelcallback of "direct" readable stream doesn't get called #18315 repro).Bun.serveanswered 200 with an empty body for such a stream since 1.2.17. Now the handler throws anderror()answers.pull: release text and instruction counts checked (Notes).Notes
Question for a maintainer: reject a missing
pullat construction (this PR), or at first use?I built and tested both. At first use, the owner of the sink gets the TypeError and the stream ends errored and locked, but the sink has acted by then:
pullgiven toBun.writeover an existing filefetchPOST bodyBun.spawnstdinBun.serveresponseerror()not callederror()runsThe first-use variant keeps
new ReadableStream({ type: "direct" })constructible. It is about 9 lines inreadDirectStreamand I can switch to it.Repro on main:
An S3 write of this stream sends a PUT with 0 bytes for the key.
Where this comes from. I found it by probing, there is no open issue. #18654 (closed, the crash it reports had another cause) attaches a
Bun.serveprogram whose source hasstart(),cancel()andtype: "direct"and nopull. Its log showscancel()calls and nostart()call. #18315 (closed) builds a direct source with onlycancel()to show acancel()bug. The repository has no other source like this: I scanned everytype: "direct"literal intest,docs,src/js,packagesandbench.History of a source without
pull(release binaries 1.2.17, 1.2.18, 1.3.0, 1.3.13, 1.3.14 and 1.4.0 to 1.4.4, Linux x64):getReader().read()Bun.servebodyfetchuploadBun.spawnstdinBun.write(file, response)HTMLRewriterread()afterBun.writeOnly
Bun.servehas a stable empty body. The other sinks changed in 1.4.Measurements. Release builds of the merge base 89452d2 and of this PR at 00f33d7. The commit after that one changes only the text of the TypeError (one string of 114 bytes). perf, valgrind and bloaty are not installed in the build container, so instructions come from
perf_event_open(instructions:u), 5 interleaved runs per build.size): 143,095,996 B on both builds. Linker map: the constructor +5 B,JSDirectStreamSource::finishCreation-7 B,consumeDirectStreamToArrayBuffer-108 B,assignToStream+81 B (the inliner now inlinesJSPromise::createthere). The TypeError string ofoneShotCallPullis gone.new ReadableStream({ type: "direct", pull }): 1387.9 to 1382.4 (run spread 2.5, 2,000,000 runs).read(): 2233.8 to 2234.6 (spread 3.6 to 6.8, 2,000,000 reads). Pernew Response(stream).bytes(): 13256.0 to 13238.2 (spread 15 to 33). PerHTMLRewriterattach: 11716.8 to 11708.0 (spread 30 to 68). All three are below the noise floor.Related open PRs. #46295 and #41561 add
hasPull()for the two guards that this PR deletes. After this PR they drop it, and the test in #46295 that expects an empty body fromBun.writemust expect the constructor TypeError. #42196 (draft) covers the sibling where the lazy start of a native stream fails.Not changed here.
pull()that throws synchronously into a native sink leaves the stream closed, not errored. The owner fails in both cases.startandpullconstructs, and Bun never calls itsstart(). Should that be an error too?callDirectPullkeeps its TypeError for a controller without a source.Self-review. It rejected the first version of this change (fail at first use, through one shared
pull()method onJSDirectStreamSource) and asked for this one. Of its 25 concerns, 19 are addressed by the new shape and this text. The other 6 are about a synchronouspull()throw inreadDirectStream(errored state, a falsy thrown value, two exits before the lock), which this PR does not touch.Suites run. Debug ASAN build:
streams.test.js(731 pass),serve-direct-readable-stream.test.ts(175),web-stream-state.test.ts(46), the spawn stdin suites,s3.test.ts(307),wpt-streams.test.ts(1175),html-rewriter.test.js(202),body.test.ts(776),node-stream.test.js(123),worker-terminate-lifetime.test.ts(27). The same files pass on the release build of this PR, withbun-write.test.js(104). On the release build of the merge base the 4 new cases fail.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js