Repository navigation
Conversation
…S thread For Bun.write(path, typedArray) with a non-S3 destination, the source buffer was converted to a Blob via Blob::get -> try_create, which copied the entire payload on the JS thread: memfd_create + ftruncate + pwrite64(len) on Linux for buffers >= 8MB, or a full memcpy into a Vec otherwise. A 500MB write stalled the event loop for ~380ms before the pool thread ever saw the data. Pin and GC-protect the ArrayBuffer and wrap its bytes in a Bytes store whose allocator free releases the pin+protect. WriteFile::then / WriteFileWindows::run_from_js_thread drop that store on the JS thread, so the borrow is safely released there. The pool-thread write reads the user's buffer directly, matching fs.promises.writeFile. S3 destinations keep the copying path since their store may outlive the JS-thread drop. Resizable buffers fall back to the copy path (a concurrent shrink would invalidate the borrowed slice).
WalkthroughChangesBun.write uses zero-copy handling for eligible ArrayBuffers on non-S3 destinations. Tests cover timing, output correctness, supported views, rejected writes, and buffer cleanup. Zero-copy ArrayBuffer writes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The 4096-byte offset view was handled by write_bytes_to_file_fast on non-Windows and never reached borrow_array_buffer_for_write. Use a 512KB window so every case in the content test routes through the borrowed path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:30 AM PT - Jul 31st, 2026
❌ @robobun, your commit 6163626 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 36535That installs a local version of the PR into your bun-36535 --bun |
Fresh 300KB buffer per iteration with Bun.gc(true), asserting protectedObjectTypeCounts stays flat across both the success path and a batch of rejected writes (EISDIR), so a missed unprotect/unpin on either path would surface here.
Both entry points route through write_file_internal but via different PathOrBlob arms; the content test now runs each input through both.
|
CI status for 6163626 (build #86173): the build finished with 195 jobs passed and 1 failed (
Correction, made after the close: an earlier version of this comment said the test "passed on every lane in all three runs" and that the PR was "ready for review". Builds #86135 and #86156 were canceled by later pushes with 6 jobs not run, so that claim holds only for #86173. The PR was not ready for review: it changes what |
The 1ms floor in Math.max(small_ms, 1) * 8 pinned the threshold at 8ms on release hardware where the 1MB sample completes in microseconds; on a fast runner the unfixed 64MB memfd snapshot can finish under that and pass. Bump the large sample to 128MB, drop the floor to 0.05ms, and widen the multiplier to 16x so the unfixed O(n) cost (128x the 1MB sample) is clearly above threshold while the fixed O(1) cost stays well below on both release and debug builds.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 4996-5034: Update borrow_array_buffer_for_write to replace the
manual data.protect/value.unprotect pair with a boxed Strong handle stored in
the allocator’s opaque pointer field. In the free callback, recover and drop
that Strong on the JS thread instead of reconstructing JSValue and calling
unprotect, while preserving the existing pin/unpin cleanup and StoreRef drop
behavior.
In `@test/js/bun/io/bun-write-typed-array-nonblocking.test.ts`:
- Around line 50-98: Change the independent subprocess tests in the affected
range, including the test beginning “Bun.write(path, typedArray) writes…”, from
test to test.concurrent. Preserve their existing bodies, assertions, and setup;
only update the test declaration so both independent subprocess tests can run
concurrently.
- Around line 59-64: Add resizable ArrayBuffer and zero-length buffer entries to
the cases in the Bun.write/BunFile.write test, ensuring both trigger the
borrow_array_buffer_for_write fallback through Blob::get(). For each new case,
assert the expected written bytes and byte counts for both APIs while preserving
the existing non-resizable view coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5eacf96-2b3a-4879-b263-617f70d1a350
📒 Files selected for processing (2)
src/runtime/webcore/Blob.rstest/js/bun/io/bun-write-typed-array-nonblocking.test.ts
… fallback The content and GC-stress tests spawn independent subprocesses with no shared state; run them concurrently. Add resizable-ArrayBuffer and empty Uint8Array cases to the content round-trip to exercise the borrow_array_buffer_for_write None branches.
There was a problem hiding this comment.
No issues found in this revision — all three earlier review threads (GC-stress coverage, sibling Bun.file().write entry point, release-build timing gate) are now addressed, and the coderabbit nits are applied or reasonably declined. Deferring rather than auto-approving because borrow_array_buffer_for_write is new cross-thread GC-lifetime machinery (pin + protect stashed in a custom allocator whose free must run on the JS thread), which is worth a maintainer's eyes even though it mirrors the existing StringOrBuffer::Buffer pattern.
Checked this pass:
- StoreRef ownership chain: on POSIX the last ref drops in
WriteFile::then(JS thread viaheap::take); on Windows inWriteFileWindows::deinitviarun_from_js_thread/on_finish(uv-loop thread = JS thread), including thethrowandWriteFileWindowsDeinitializederror arms. Bytes::drop→StdAllocator::freeskips empty slices, so thebyte_len == 0early-out is load-bearing (otherwise unpin/unprotect would never fire) — correctly handled.JSValue(usize)↔*mut c_voidround-trip in the allocator ptr is bit-identical;SizeType = u64sobyte_len as SizeTypedoesn't truncate.- Resizable guard matches the
NodeHTTPResponseprecedent (pin doesn't preventresize()'s PROT_NONE); non-growable SAB is fine to borrow.
Extended reasoning...
Overview
Adds borrow_array_buffer_for_write in src/runtime/webcore/Blob.rs (~40 lines) plus a 6-line call site inside write_file_internal's source-blob construction, gated on !destination_blob.is_s3(). When data is a non-empty, non-resizable ArrayBuffer/view, it pins the backing JSC::ArrayBuffer, GC-protects the JSValue, and builds a store::Bytes whose StdAllocator::free releases both — so the pool-thread do_write_loop reads the user's buffer directly instead of the old Blob::get snapshot (memfd pwrite64 or .to_vec() on the JS thread). New test file covers timing scaling, content round-trip across Uint8Array/ArrayBuffer/DataView/offset-view/resizable/empty × Bun.write/Bun.file().write, and a 64-success + 16-EISDIR GC-stress loop asserting protectedObjectTypeCounts stays at baseline.
Security risks
None. No parsing of untrusted input, no auth/crypto/permissions surface. The change is purely a lifetime/ownership optimization on an existing write path.
Level of scrutiny
High. This is new unsafe code that (a) hands a JS-heap-backed slice to a pool thread, (b) relies on a custom allocator free callback firing on the JS thread as the last-ref release point, and (c) encodes a JSValue into a *mut c_void ptr slot. REVIEW.md calls memory safety "the most-blocked category" and specifically asks reviewers to be able to answer "who frees this, when, on which paths" and to prove GC-safety with a stress test. I traced the StoreRef graph on both platforms and both success/error arms and it checks out — the JS-thread refs (source_blob scopeguard, source_store local) drop first, leaving WriteFile.bytes_blob / WriteFileWindows.bytes_blob as the sole owner, which is dropped in then / deinit on the JS thread. The pool thread only calls shared_view() (a &[u8] borrow, no ref-count change). But this is exactly the class of change where a maintainer familiar with the WriteFile task lifecycle (VM shutdown mid-task, worker termination, the io-request path when close_after_io is set) should confirm the invariant holds on every terminal path.
Other factors
- The implementation deliberately mirrors the existing
fs.promises.writeFilepattern (MarkedArrayBuffer::from_js_pinned+value.protect()insrc/runtime/node/types.rs, released byUnprotect::unprotect), which is a strong precedent. - The one open design point is coderabbit's
Strong-vs-protect()suggestion, which the author declined with a reasonable rationale (matches sibling path, avoids a box, pin still needs separate carriage) and left open to maintainer preference. - All three of my earlier inline comments are addressed in fed3e45 / 11c0e6b / 72ab7d0, and CI on the latest push (#86173) is building; #86156 was green on this test file across all lanes.
- The timing test's 0.8 ms release threshold (0.05 ms floor × 16) has ~40× headroom vs. the observed 0.02 ms fixed-build cost per the author's local measurements; best-of-3 in an isolated subprocess should keep it stable, but it's the one assertion worth watching on noisy shared runners.
|
Closing this PR because it has been inactive for more than 90 days. |
|
The 90-day stale sweep closed this PR. I corrected the PR body after the close. The changes:
Do not revive this branch as it is. The stall still reproduces on 1.4.3 (486 ms for a 512 MB source). A review of where the fix belongs is in progress, and any new PR follows from it. |
Status: closed unmerged by the 90-day stale sweep. Body corrected after the close. Read Downsides before any revival.
Problem
Bun.write(path, typedArray)copies the whole source on the JS thread before the pooled write starts: 50 to 57 ms for 128 MB.fs.promises.writeFilereturns in about 2.5 ms.write_file_internal->Blob::get->Blob::try_create(src/runtime/webcore/Blob.rs): a memfd copy on Linux for 8 MB or more,bytes.to_vec()otherwise.Fix
borrow_array_buffer_for_writepins and GC-protects the source and wraps its bytes in aBytesstore. The allocatorfreeof the store releases both.WriteFile::thenandWriteFileWindows::run_from_js_threaddrop the store on the JS thread.test/js/bun/io/bun-write-typed-array-nonblocking.test.ts.Background
JSC::ArrayBuffer::pin) stops JS from detaching a buffer.protect()is a GC root.Downsides
const p = Bun.write(out, buf); buf.fill(x); await pwrites the old bytes (checked on 1.4.3). With this change the pool thread readsbufafter the call returns, so the new bytes can reach the file.WebAssembly.Memory:grow()detaches it. This branch has no guard for that. Main has one since Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021 (PinnedArrayBuffer::root_read_only).Notes
What was measured, and on which build. All on one linux x64 container.
Synchronous part of the call (time until the promise is returned), 128 MB source:
BUN_FEATURE_FLAG_DISABLE_MEMFD=1Largest gap of a 1 ms interval during
await Bun.write(out, 256 MB):Bun.writefs.promises.writeFileThere is no release "after" number for the second table.
Measured after the close, on release 1.4.3 (rev 367d939), which does not have this change:
Bun.writecall time 31 to 51 ms at 128 MB and 486 ms at 512 MB.fs.promises.writeFilecall time 2.4 to 3.3 ms at 32, 128 and 512 MB.Corrections to the first version of this body:
stracelines as console output and said "a 500 MB write stalled a 1 ms heartbeat for ~378 ms". Both are from the report that started this work.straceis not installed in the container that built this change.Also: the gate is "destination is not S3", not "destination is a file". The change adds an
unsafeblock and an allocatorfreethat must run on the JS thread.Not checked:
fs.promises.writeFile, which reads in place in the same way, wrote the later bytes in that probe.WebAssembly.Memorycase on a build of this branch. On 1.4.3 (rev 367d939, before Hardening: fix crashes across the runtime, bundler, dev server, shell and CLI #46021)fs.promises.writeFile(out, viewOfWasmMemory)followed bymemory.grow(1)wrote a file with zero pages at its end in 2 of 24 runs with 16 or more other live memories, and in 0 of 12 runs with 8 or fewer.CI: build #86173 (head 6163626) finished with 195 jobs passed and 1 failed (
debian 13 aarch64 - test-bun). The test of this change is in neither the failed nor the flaky list of that build.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 0 rejected · iteration 2
evidence per changed file