Repository navigation
Conversation
The ReadFile work-pool task read the file bytes off-thread but handed
the raw buffer back to the JS-thread completion handler, which then ran
the full UTF-8 -> UTF-16 decode (strings::to_utf16_alloc) inline. On a
large file of invalid UTF-8 this froze the event loop for the entire
decode; a 500 MB random file held the loop for ~11 s even though
fs.promises.readFile(path, 'utf8') on the same runtime offloaded the
identical decode and kept the max loop gap at ~30 ms.
Give ReadFileToJs a WANTS_TEXT associated const and, when set, run
decode_blob_text_owned (BOM handling + to_bun_string_from_owned_slice,
the same helper readFile('utf8') uses) at the tail of do_read_loop,
still on the work pool. ReadFileRead now carries an optional pre-built
BunString; NewReadFileHandler wraps it with transfer_to_js instead of
calling to_string_with_bytes. POSIX only for now: ReadFileUV on
Windows delivers bytes on the uv loop thread.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 2:47 AM PT - Jul 31st, 2026
✅ @robobun, your commit 7fd4282410b8853c1dc180a06a7464e6bfe7eda2 passed in 🧪 To try this PR locally: bunx bun-pr 36536That installs a local version of the PR into your bun-36536 --bun |
There was a problem hiding this comment.
No bugs found; the previous test-flakiness concern was addressed in 7fd4282. Deferring to a human for the native cross-thread BunString handoff and the duplicated BOM/decode logic in decode_blob_text_owned (marked "keep in sync" with to_string_with_bytes).
What was reviewed:
decode_blob_text_ownedmirrorsto_string_with_bytes::<Temporary>for empty/BOM/UTF-16LE/UTF-8 — validated against the 7-variant correctness test that compares to in-memoryBlob#text().- Off-thread
to_bun_string_from_owned_slicematches the establishedfs.promises.readFilepattern (node_fs.rs:7115insidework_pool_callback); the resultingWTFStringImplhas an atomic refcount so the work-pool→JS-thread handoff is sound. - Ownership on the
decoded_textpath:self.bufferistake()'n before decode, soReadFileRead::bufis an empty box that the handler reclaims viaheap::take;is_dead()is checked and throws OOM beforetransfer_to_js.
Extended reasoning...
Overview
Moves the UTF-8→UTF-16 decode for Bun.file(path).text() from the JS thread to the work pool (POSIX only). Adds a WANTS_TEXT const to ReadFileToJs, threads a convert_to_text: bool through ReadFile::create[_with_ctx], runs a new decode_blob_text_owned() at the tail of do_read_loop(), and carries the resulting BunString through ReadFileRead::decoded_text for NewReadFileHandler::run to transfer_to_js on the JS thread. ReadFileUV (Windows) is unchanged and always sets decoded_text: None. ~80 lines of native change across Blob.rs and blob/read_file.rs, plus a 7-variant correctness matrix and a heartbeat regression test.
Security risks
None identified. No new user-controlled input paths; the decode is the same strings::to_utf16_alloc / BunString::clone_utf16 machinery already used by both the on-thread path and fs.promises.readFile.
Level of scrutiny
High. This is native memory-safety code in the most-blocked review category: cross-thread BunString lifetime, raw *mut [u8] ownership contracts in ReadFileRead, and a new struct field (decoded_text) that must be correctly take()'n on every terminal path (then() success, then() error, drop). The pattern is sound and follows NodeFS::read_file's precedent, but a maintainer should confirm the design choice to duplicate the BOM/UTF-16LE handling in decode_blob_text_owned (with a "keep in sync" comment) rather than refactor to_string_with_bytes to share a helper — REVIEW.md's "one source of truth" guidance applies directly here.
Other factors
- CI build #86118 was still running at review time; the PR description itself notes "no test proof · Deferring to CI".
- The
blob.sizeupdate is skipped on thedecoded_textbranch inNewReadFileHandler::run, butblobthere is a local moved fromhandler.contextandto_string_with_bytesnever readsself.size, so this is not a behavior change. - Windows keeps the on-thread decode; the heartbeat test is correctly
skipIf(isWindows). - My earlier flakiness note (unbounded-below
durin the ratio assertion) was addressed with adur < 50floor guard in 7fd4282.
|
Closing this PR because it has been inactive for more than 90 days. |
|
The stale bot closed this PR after 90 days. No maintainer reviewed it. CI was green on 7fd4282 (build 86118). The bug is still present:
This branch cannot be revived as it is:
I am checking the design against current main, Windows included. I will post the result here. |
|
The result of that check: #47509 replaces this PR.
|
What
await Bun.file(path).text()ran the UTF-8→UTF-16 decode synchronously on the JS thread after the off-thread read finished, so a large non-ASCII file froze the event loop for the entire decode.fs.promises.readFile(path, 'utf8')in the same runtime already offloads the identical decode.Repro
On main, 100 MB of random bytes:
dur_ms: 1456, max_loop_gap_ms: 1411. The same file viafs.promises.readFile(p, 'utf8'):dur_ms: 1468, max_loop_gap_ms: 6. Scales linearly; 500 MB holds the loop for ~11 s.Cause
ReadFile::run()fillsself.buffer: Vec<u8>on the work pool.ReadFile::then()(JS thread) hands the raw bytes toNewReadFileHandler, which callsBlob::to_string_with_bytes→strings::to_utf16_alloc(buf, false, false)inline.NodeFS::read_fileforreadFile(p, 'utf8')instead runswebcore::encoding::to_bun_string_from_owned_sliceinsideAsyncFSTask::work_pool_callback, so the JS-thread step is justBunString::transfer_to_js.Fix
Add
ReadFileToJs::WANTS_TEXT(true forToStringWithBytesFn). When set,do_read_looprunsdecode_blob_text_owned(the same BOM handling asto_string_with_bytes::<Temporary>, thento_bun_string_from_owned_slice(.., Utf8)) at its tail, still on the work pool, and stashes the resultingBunString.ReadFileReadcarries it through; the JS-thread handler justtransfer_to_js's it.POSIX only.
ReadFileUVon Windows delivers bytes on the uv loop thread, so the decode still happens inline there; routing that through the work pool is a larger change.Verification
100 MB random bytes, debug+ASAN:
max_loop_gap_msdrops from the full decode time to ~40 ms.bun-file.test.tsadds a heartbeat probe (max gap must be < half the total duration; before the fix it was ~97%) plus correctness checks thatBun.file(p).text()matchesnew Blob([bytes]).text()across ASCII, multibyte UTF-8, UTF-8 BOM, UTF-16LE BOM, invalid UTF-8, BOM-only, and empty inputs.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/bun/util/bun-file.test.ts