Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/jsc/webcore_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,13 @@ pub mod store {
pub mime_type: MimeType,
pub ref_count: bun_ptr::ThreadSafeRefCount<Store>,
pub is_all_ascii: Option<bool>,
/// JS-thread-only. For an fd-backed `File` store, points at the
/// `ReadFile` currently draining the fd (set before scheduling,
/// cleared in `ReadFile::then`). A second `.arrayBuffer()`/`.text()`
/// on the same store attaches to it instead of spawning a racing
/// reader — two readers on one pipe fd split the byte stream and the
/// loser's `epoll_ctl(ADD)` fails with `EEXIST`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub in_flight_blob_reader: core::sync::atomic::AtomicPtr<core::ffi::c_void>,
}

impl Default for Store {
Expand All @@ -526,6 +533,7 @@ pub mod store {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}
}
}
Expand Down Expand Up @@ -882,6 +890,7 @@ pub mod store {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}))
}

Expand Down
1 change: 1 addition & 0 deletions src/runtime/api/BunObject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3034,6 +3034,7 @@ mod stdio_stores {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
});
StoreRef::from(store)
}
Expand Down
1 change: 1 addition & 0 deletions src/runtime/api/standalone_graph_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ impl FileJsc for File {
mime_type: MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}));
// make it never free
store.ref_();
Expand Down
1 change: 1 addition & 0 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5388,6 +5388,7 @@ pub(crate) fn __bun_stdio_blob_store_new(
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init_exact_refs(2),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
});
bun_core::heap::into_raw(store).cast()
}
Expand Down
52 changes: 42 additions & 10 deletions src/runtime/webcore/Blob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,17 +472,37 @@ impl BlobExt for Blob {

#[cfg(not(windows))]
{
let store = self.store().expect("infallible: store present").clone();

// For fd-backed stores (Bun.stdin, Bun.file(fd)), a second
// concurrent blob read would race `read()` on the shared fd and
// re-register it with epoll (EEXIST). Attach to the in-flight
// reader instead so every caller sees the same full byte stream.
Comment thread
robobun marked this conversation as resolved.
Outdated
if read_file::ReadFile::try_coalesce_fd_read(
&store,
handler.cast::<c_void>(),
read_file::completion_thunk::<Handler<'_, F>>,
) {
// SAFETY: handler was just boxed; sole owner.
unsafe { (*handler).promise = jsc::JSPromiseStrong::init(global) };
// SAFETY: same `handler` as above; still solely owned here.
let promise_value = unsafe { (*handler).promise.value() };
promise_value.ensure_still_alive();
debug!("doReadFile: coalesced onto in-flight fd reader");
return promise_value;
Comment thread
robobun marked this conversation as resolved.
}

let file_read = read_file::ReadFile::create(
self.store().expect("infallible: store present").clone(),
store.clone(),
self.offset.get(),
self.size.get(),
handler,
)
.unwrap_or_else(|e| bun_core::handle_oom(Err(e)));
let read_file_task = read_file::ReadFileTask::create_on_js_thread(
global,
bun_core::heap::into_raw(file_read),
);
let file_read_ptr = bun_core::heap::into_raw(file_read);
read_file::ReadFile::mark_in_flight(&store, file_read_ptr);
let read_file_task =
read_file::ReadFileTask::create_on_js_thread(global, file_read_ptr);

// Create the Promise only after the store has been ref()'d.
// The garbage collector runs on memory allocations
Expand Down Expand Up @@ -688,18 +708,26 @@ impl BlobExt for Blob {
}
#[cfg(not(windows))]
{
let store = self.store().expect("infallible: store present").clone();
if read_file::ReadFile::try_coalesce_fd_read(
&store,
ctx.cast::<c_void>(),
NewInternalReadFileHandler::<C, F>::run,
) {
return;
}
let file_read = read_file::ReadFile::create_with_ctx(
self.store().expect("infallible: store present").clone(),
store.clone(),
ctx.cast::<c_void>(),
NewInternalReadFileHandler::<C, F>::run,
self.offset.get(),
self.size.get(),
)
.unwrap_or_else(|e| bun_core::handle_oom(Err(e)));
let read_file_task = read_file::ReadFileTask::create_on_js_thread(
global,
bun_core::heap::into_raw(file_read),
);
let file_read_ptr = bun_core::heap::into_raw(file_read);
read_file::ReadFile::mark_in_flight(&store, file_read_ptr);
let read_file_task =
read_file::ReadFileTask::create_on_js_thread(global, file_read_ptr);
// SAFETY: `read_file_task` was just heap-allocated by `create_on_js_thread`.
read_file::ReadFileTask::schedule(unsafe { &mut *read_file_task });
}
Expand Down Expand Up @@ -2486,6 +2514,9 @@ impl BlobExt for Blob {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(
core::ptr::null_mut(),
),
}));
let blob = Blob::init_with_store(store, global_this);
if was_string && blob.content_type_slice().is_empty() {
Expand Down Expand Up @@ -5615,6 +5646,7 @@ pub fn jsdom_file_construct_(
ref_count: bun_ptr::ThreadSafeRefCount::init(),
mime_type: bun_http_types::MimeType::NONE,
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}))));
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/webcore/blob/Store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ impl StoreExt for Store {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}))
}

Expand All @@ -159,6 +160,7 @@ impl StoreExt for Store {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}))
}

Expand All @@ -171,6 +173,7 @@ impl StoreExt for Store {
mime_type: bun_http_types::MimeType::NONE,
ref_count: bun_ptr::ThreadSafeRefCount::init(),
is_all_ascii: None,
in_flight_blob_reader: core::sync::atomic::AtomicPtr::new(core::ptr::null_mut()),
}))
}

Expand Down
122 changes: 101 additions & 21 deletions src/runtime/webcore/blob/read_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,17 @@ impl<'a, F: ReadFileToJs> ReadFileCompletion for NewReadFileHandler<'a, F> {

pub type ReadFileOnReadFileCallback = fn(ctx: *mut c_void, bytes: ReadFileResultType);

/// Monomorphized `ReadFileCompletion::run` thunk matching
/// [`ReadFileOnReadFileCallback`], so the same `(callback, ctx)` pair can be
/// stored as the primary completion or pushed onto `extra_completions`.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[cfg(not(windows))]
pub fn completion_thunk<C: ReadFileCompletion>(ctx: *mut c_void, bytes: ReadFileResultType) {
// SAFETY: `ctx` is the `*mut C` the caller passed through
// `on_complete_ctx`/`extra_completions`; ownership transfers per
// `ReadFileCompletion::run`.
let _ = unsafe { C::run(ctx.cast::<C>(), bytes) };
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

pub struct ReadFileRead {
/// Always a `Box::<[u8]>::into_raw` from the producer's read buffer
/// (`Vec::into_boxed_slice()` so layout is exactly `(ptr, len)`). Every
Expand Down Expand Up @@ -196,6 +207,12 @@ pub struct ReadFile {
pub errno: Option<Error>,
pub on_complete_ctx: *mut c_void,
pub on_complete_callback: ReadFileOnReadFileCallback,
/// Additional `(callback, ctx)` pairs attached by concurrent reads on the
/// same fd-backed store (see `Store::in_flight_blob_reader`). Pushed on
/// the JS thread while the read runs on the work pool; drained in
/// `then()` on the JS thread. Guarded so the JS-thread push does not
/// alias the work-pool `&mut self`.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub extra_completions: bun_threading::Guarded<Vec<(ReadFileOnReadFileCallback, *mut c_void)>>,
pub io_task: Option<*mut ReadFileTask>,
pub io_poll: io::Poll,
pub io_request: io::Request,
Expand Down Expand Up @@ -361,6 +378,7 @@ impl ReadFile {
errno: None,
on_complete_ctx: on_read_file_context,
on_complete_callback,
extra_completions: bun_threading::Guarded::new(Vec::new()),
io_task: None,
io_poll: io::Poll::default(),
io_request: io::Request {
Expand All @@ -383,27 +401,57 @@ impl ReadFile {
context: *mut C,
) -> Result<Box<ReadFile>, Error> {
// `ReadFileCompletion`
// monomorphizes per `C`, so `handler_run::<C>` calls `C::run` directly
// monomorphizes per `C`, so `completion_thunk::<C>` calls `C::run` directly
// and `on_complete_ctx` is the unwrapped `*mut C` — no extra heap box,
// nothing to leak on the `Err` path.
fn handler_run<C: ReadFileCompletion>(ctx: *mut c_void, bytes: ReadFileResultType) {
// The JsTerminated error is intentionally swallowed.
// TODO: propagate the exception.
// SAFETY: `ctx` is the `*mut C` passed unmodified through
// `on_complete_ctx`; ownership transfers per `ReadFileCompletion::run`.
let _ = unsafe { C::run(ctx.cast::<C>(), bytes) };
}
ReadFile::create_with_ctx(
store,
context.cast::<c_void>(),
handler_run::<C>,
completion_thunk::<C>,
off,
max_len,
)
}

pub const IO_TAG: io::Tag = io::Tag::ReadFile;

/// JS-thread-only. If `store` is fd-backed and already has a `ReadFile`
/// in flight, push `(callback, ctx)` onto its `extra_completions` so the
/// caller's promise resolves with the same bytes, and return `true`.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[cfg(not(windows))]
pub fn try_coalesce_fd_read(
store: &StoreRef,
ctx: *mut c_void,
callback: ReadFileOnReadFileCallback,
) -> bool {
if !matches!(&store.data, Data::File(f) if f.pathlike.is_fd()) {
return false;
}
let existing = store
.in_flight_blob_reader
.load(core::sync::atomic::Ordering::Acquire);
if existing.is_null() {
return false;
}
// SAFETY: `existing` was stored on the JS thread before scheduling and
// is cleared on the JS thread in `then()` before the `ReadFile` is
// dropped; we are on the JS thread, so the pointee is live.
let existing = unsafe { &*(existing.cast::<ReadFile>()) };
existing.extra_completions.lock().push((callback, ctx));
true
Comment thread
robobun marked this conversation as resolved.
Outdated
}
Comment thread
robobun marked this conversation as resolved.

/// JS-thread-only. Publish `reader` as the in-flight reader on `store` so
/// subsequent fd-backed blob reads coalesce onto it.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[cfg(not(windows))]
pub fn mark_in_flight(store: &StoreRef, reader: *mut ReadFile) {
if matches!(&store.data, Data::File(f) if f.pathlike.is_fd()) {
store
.in_flight_blob_reader
.store(reader.cast(), core::sync::atomic::Ordering::Release);
}
}
Comment thread
robobun marked this conversation as resolved.

pub fn on_ready(&mut self) {
bloblog!("ReadFile.onReady");
self.task = WorkPoolTask {
Expand Down Expand Up @@ -562,31 +610,51 @@ impl ReadFile {
let cb = this.on_complete_callback;
let cb_ctx = this.on_complete_ctx;

let mut this = this;

// Clear the in-flight marker first so a read scheduled from a
// completion callback starts fresh instead of attaching to this
// (about-to-drop) ReadFile, and so a later read after an error does
// not see a stale pointer.
Comment thread
robobun marked this conversation as resolved.
Outdated
let self_ptr = core::ptr::from_ref::<ReadFile>(&this)
.cast_mut()
.cast::<c_void>();
if let Some(store) = this.store.as_deref() {
let _ = store.in_flight_blob_reader.compare_exchange(
self_ptr,
core::ptr::null_mut(),
core::sync::atomic::Ordering::AcqRel,
core::sync::atomic::Ordering::Relaxed,
);
}
let extras: Vec<_> = core::mem::take(&mut *this.extra_completions.lock());

if this.store.is_none() && this.system_error.is_some() {
let mut this = this;
let system_error = this.system_error.take().unwrap();
drop(this);
for (extra_cb, extra_ctx) in extras {
extra_cb(extra_ctx, ReadFileResultType::Err(system_error.clone()));
}
cb(cb_ctx, ReadFileResultType::Err(system_error));
return Ok(());
} else if this.store.is_none() {
drop(this);
if cfg!(debug_assertions) {
panic!("assertion failure - store should not be null");
}
cb(
cb_ctx,
ReadFileResultType::Err(SystemError {
code: BunString::static_("INTERNAL_ERROR").into(),
message: BunString::static_("assertion failure - store should not be null")
.into(),
syscall: BunString::static_("read").into(),
..Default::default()
}),
);
let err = SystemError {
code: BunString::static_("INTERNAL_ERROR").into(),
message: BunString::static_("assertion failure - store should not be null").into(),
syscall: BunString::static_("read").into(),
..Default::default()
};
for (extra_cb, extra_ctx) in extras {
extra_cb(extra_ctx, ReadFileResultType::Err(err.clone()));
}
cb(cb_ctx, ReadFileResultType::Err(err));
return Ok(());
}

let mut this = this;
let _store = this.store.take().unwrap();
// reshaped for borrowck — take buffer out so it survives `drop(this)`.
let buf = core::mem::take(&mut this.buffer);
Expand All @@ -597,12 +665,24 @@ impl ReadFile {
drop(this);

if let Some(err) = system_error {
for (extra_cb, extra_ctx) in extras {
extra_cb(extra_ctx, ReadFileResultType::Err(err.clone()));
}
cb(cb_ctx, ReadFileResultType::Err(err));
return Ok(());
}

// The receiver takes ownership. Normalize to `Box<[u8]>` so every
// consumer can reclaim via `heap::take` with a matching layout.
for (extra_cb, extra_ctx) in extras {
extra_cb(
extra_ctx,
ReadFileResultType::Result(ReadFileRead {
buf: bun_core::heap::into_raw(buf.clone().into_boxed_slice()),
total_size,
}),
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
cb(
cb_ctx,
ReadFileResultType::Result(ReadFileRead {
Expand Down
Loading
Loading