Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
6 changes: 6 additions & 0 deletions src/bun_core/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2687,6 +2687,12 @@ impl BufferedStdin {
self
}

/// `true` when the next `read_byte` is served from the buffer and does not touch the fd.
#[inline]
pub fn has_buffered(&self) -> bool {
self.start < self.end
}

/// Fill `dest` from the buffer, refilling from
/// the underlying fd until `dest` is full or EOF. Returns `Ok(0)` on EOF.
///
Expand Down
26 changes: 26 additions & 0 deletions src/jsc/PosixSignalHandle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,32 @@ impl PosixSignalHandle {
event_loop.enqueue_task(task);
}
}

#[cfg(unix)]
fn main_thread() -> Option<bun_ptr::BackRef<Self>> {
let vm = VirtualMachine::get_main_thread_vm()?;
// SAFETY: `vm` and its event loop are process-lifetime; only the
// `signal_handler` slot is read.
unsafe { (*(*vm).event_loop()).signal_handler }
}

/// `true` when a signal handler has queued a signal the main thread has not consumed.
#[cfg(unix)]
pub fn has_queued() -> bool {
Self::main_thread().is_some_and(|handler| !handler.ring.is_empty())
}

/// Runs the JS listeners for every queued signal now, for a host function that
/// blocks the JS thread (`prompt()`). Main thread only, the ring's single consumer.
Comment thread
robobun marked this conversation as resolved.
Resolved
#[cfg(unix)]
pub fn run_queued_from_js_thread(global_object: &JSGlobalObject) {
let Some(handler) = Self::main_thread() else {
return;
};
while let Some(signal) = handler.ring.dequeue() {
PosixSignalTask::run_from_js_thread(signal, global_object);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Signal listeners now run inside whatever AsyncLocalStorage or Bun.ModuleGraph context the dialog's caller was in, not the event-loop context they get on the base branch. run_queued_from_js_thread at src/jsc/PosixSignalHandle.rs:79 calls the listener while $asyncContext still holds the caller's frame, and nothing saves or resets it. So getStore() in the listener returns the caller's store, and a throw in it is routed to that graph's onError through currentModuleGraph. Fix: run the queued listeners (and the drain at :84) under the event-loop async context, the value drainMicrotasks resets to at ZigGlobalObject.cpp:3128, then restore the caller's frame.

Why this was flagged

Script: als.run(store, () => prompt("name?")) with process.on("SIGTERM", () => log(als.getStore())), or the same prompt() call made from a Bun.ModuleGraph script. SIGTERM during the wait reaches wait_for_stdin (src/runtime/webcore/prompt.rs:31) -> run_queued_from_js_thread -> PosixSignalTask::run_from_js_thread (src/jsc/PosixSignalHandle.rs:79) -> innerInvokeEventListeners, a plain JSC::call (src/jsc/bindings/webcore/EventEmitter.cpp:260) with no AsyncContextFrame wrapping. The listener reads $asyncContext field 0, which is still the caller's frame, so getStore() returns store. If the listener throws, Bun__ModuleGraph__handleUncaughtException (src/jsc/VirtualMachine.rs:2105) picks the graph via currentModuleGraph, which reads the same field, so the error goes to that graph's onError instead of the process-wide path. On base the listener ran as a PosixSignalTask with ContextId::NONE after a checkpoint had reset the frame to moduleGraphAsyncContextAtEventLoop (src/jsc/bindings/ZigGlobalObject.cpp:3127-3128), so getStore() was undefined and the throw went to the host. The reset in drainMicrotasks does not help here because vm.entryScope is set while prompt() is on the stack.

Verification: wait_for_stdin (src/runtime/webcore/prompt.rs:31) calls PosixSignalHandle::run_queued_from_js_thread -> innerInvokeEventListeners, a plain call(...) at src/jsc/bindings/webcore/EventEmitter.cpp:260 with no AsyncContextFrame. drain_microtasks_and_next_ticks only resets m_asyncContextData[0] if (!vm.entryScope) (ZigGlobalObject.cpp:3125-3128), and script is on the stack here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changed here. I documented it in the PR (Downsides and notes). The listener runs synchronously inside the dialog's caller, so it sees that caller's AsyncLocalStorage store and Bun.ModuleGraph context. A fix has to install the event-loop context around the listener call and restore the caller's frame, next to makeContextCurrent and noteEnteredFromEventLoop in ModuleGraph.cpp. I did not want to add that to this PR without a maintainer's decision on which context such a listener belongs to.

}
Comment thread
robobun marked this conversation as resolved.
Resolved
}
Comment thread
robobun marked this conversation as resolved.
Resolved
}

/// This is the signal handler entry point. Calls enqueue on the ring buffer.
Expand Down
98 changes: 83 additions & 15 deletions src/runtime/webcore/prompt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,62 @@ use bun_core::EncodedSlice;
use bun_core::Output;
use bun_jsc::EncodedSliceJsc as _;

/// Waits until stdin is readable and runs queued JS signal listeners meanwhile.
/// Signal handlers are SA_RESTART, so `read(2)` never sees EINTR; `pselect(2)`
/// does, and the mask closes the gap between the queue check and the wait.
Comment thread
robobun marked this conversation as resolved.
Resolved
#[cfg(unix)]
fn wait_for_stdin(global: &JSGlobalObject) {
use bun_jsc::PosixSignalHandle;

let fd = bun_sys::Fd::stdin().native();
loop {
PosixSignalHandle::run_queued_from_js_thread(global);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

// SAFETY: zeroed `sigset_t` and `fd_set` are valid inputs to
// `sigfillset` and `FD_SET`, which fill them; `fd` is stdin, below
// FD_SETSIZE; `previous` is written by `pthread_sigmask` before
// `pselect` reads it.
let interrupted = unsafe {
let mut all: libc::sigset_t = core::mem::zeroed();
let mut previous: libc::sigset_t = core::mem::zeroed();
libc::sigfillset(&mut all);
libc::pthread_sigmask(libc::SIG_BLOCK, &all, &mut previous);

let interrupted = PosixSignalHandle::has_queued() || {
let mut readable: libc::fd_set = core::mem::zeroed();
libc::FD_SET(fd, &mut readable);
let rc = libc::pselect(
fd + 1,
&mut readable,
core::ptr::null_mut(),
core::ptr::null_mut(),
core::ptr::null(),
&previous,
);
rc < 0 && bun_sys::last_errno() == libc::EINTR
Comment thread
robobun marked this conversation as resolved.
Resolved
};

libc::pthread_sigmask(libc::SIG_SETMASK, &previous, core::ptr::null_mut());
interrupted
};
if !interrupted {
return;
}
Comment thread
robobun marked this conversation as resolved.
Resolved
}
Comment thread
robobun marked this conversation as resolved.
Resolved
}

#[cfg(not(unix))]
fn wait_for_stdin(_global: &JSGlobalObject) {}

/// `alert()` and `confirm()` read unbuffered, so every byte waits on the fd.
fn take_byte(
global: &JSGlobalObject,
reader: &mut bun_core::output::StdinReader,
) -> bun_core::CrateResult<u8> {
wait_for_stdin(global);
reader.take_byte()
}

/// https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-alert
#[bun_jsc::host_fn(export = "WebCore__alert")]
fn alert(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
Expand Down Expand Up @@ -51,7 +107,9 @@ fn alert(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
// 7. Optionally, pause while waiting for the user to acknowledge the message.
let mut reader = Output::stdin_reader();
loop {
let Ok(byte) = reader.take_byte() else { break };
let Ok(byte) = take_byte(global, &mut reader) else {
break;
};
if byte == b'\n' {
break;
}
Expand Down Expand Up @@ -105,7 +163,7 @@ fn confirm(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
// 6. Pause until the user responds either positively or negatively.
let mut reader = Output::stdin_reader();

let Ok(first_byte) = reader.take_byte() else {
let Ok(first_byte) = take_byte(global, &mut reader) else {
return Ok(JSValue::FALSE);
};

Expand All @@ -116,7 +174,7 @@ fn confirm(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
match first_byte {
b'\n' => return Ok(JSValue::FALSE),
b'\r' => {
let Ok(next_byte) = reader.take_byte() else {
let Ok(next_byte) = take_byte(global, &mut reader) else {
// They may have said yes, but the stdin is invalid.
return Ok(JSValue::FALSE);
};
Expand All @@ -125,7 +183,7 @@ fn confirm(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
}
}
b'y' | b'Y' => {
let Ok(next_byte) = reader.take_byte() else {
let Ok(next_byte) = take_byte(global, &mut reader) else {
// They may have said yes, but the stdin is invalid.

return Ok(JSValue::FALSE);
Expand All @@ -137,7 +195,7 @@ fn confirm(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
return Ok(JSValue::TRUE);
} else if next_byte == b'\r' {
// Check Windows style
let Ok(second_byte) = reader.take_byte() else {
let Ok(second_byte) = take_byte(global, &mut reader) else {
return Ok(JSValue::FALSE);
};
if second_byte == b'\n' {
Expand All @@ -148,7 +206,7 @@ fn confirm(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
_ => {}
}

while let Ok(b) = reader.take_byte() {
while let Ok(b) = take_byte(global, &mut reader) {
if b == b'\n' || b == b'\r' {
break;
}
Expand All @@ -172,17 +230,26 @@ pub mod prompt {
}

/// Small trait exposing `read_byte() -> Result<u8, _>`; the only
/// concrete impl is the process-global `BufferedStdin`.
/// concrete impl is [`InterruptibleStdin`].
pub trait ReadByte {
type Error;
fn read_byte(&mut self) -> Result<u8, Self::Error>;
}

impl ReadByte for bun_core::output::BufferedStdin {
/// The process-global `BufferedStdin`; calls [`wait_for_stdin`] before each refill.
pub(crate) struct InterruptibleStdin<'a> {
global: &'a JSGlobalObject,
reader: &'a mut bun_core::output::BufferedStdin,
}

impl ReadByte for InterruptibleStdin<'_> {
type Error = bun_core::Error;
#[inline]
fn read_byte(&mut self) -> Result<u8, Self::Error> {
bun_core::output::BufferedStdin::read_byte(self)
if !self.reader.has_buffered() {
wait_for_stdin(self.global);
}
self.reader.read_byte()
Comment thread
robobun marked this conversation as resolved.
Resolved
}
}

Expand Down Expand Up @@ -306,8 +373,10 @@ pub mod prompt {
// `bun.Output.buffered_stdin.reader()` — process-global 4 KiB buffered stdin.
// SAFETY: process-global static; prompt() runs single-threaded on the JS
// main thread, so the exclusive borrow is sound for this scope.
let reader: &mut bun_core::output::BufferedStdin =
unsafe { &mut *Output::buffered_stdin_reader() };
let mut reader = InterruptibleStdin {
global,
reader: unsafe { &mut *Output::buffered_stdin_reader() },
};
let mut second_byte: Option<u8> = None;
let Ok(first_byte) = reader.read_byte() else {
// 8. Let result be null if the user aborts, or otherwise the string
Expand Down Expand Up @@ -341,7 +410,7 @@ pub mod prompt {
// size to 4096. If that is too small, then just dynamically allocate
// the rest.
if let Err(e) = read_until_delimiter_array_list_append_assume_capacity(
&mut *reader,
&mut reader,
&mut input,
b'\n',
2048,
Expand All @@ -355,7 +424,7 @@ pub mod prompt {
input.ensure_total_capacity(4096);

if let Err(e2) = read_until_delimiter_array_list_append_assume_capacity(
&mut *reader,
&mut reader,
&mut input,
b'\n',
4096,
Expand All @@ -366,8 +435,7 @@ pub mod prompt {
return Ok(JSValue::NULL);
}

if read_until_delimiter_array_list_infinity(&mut *reader, &mut input, b'\n')
.is_err()
if read_until_delimiter_array_list_infinity(&mut reader, &mut input, b'\n').is_err()
{
// 8. Let result be null if the user aborts, or otherwise the string
// that the user responded with.
Expand Down
6 changes: 6 additions & 0 deletions src/threading/signal_ring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ impl<const CAPACITY: usize> SignalRing<CAPACITY> {
}
}

/// `true` when no producer has claimed a slot since the last `dequeue`.
pub fn is_empty(&self) -> bool {
let (head, tail) = unpack(self.state.load(Ordering::Acquire));
head == tail
}

/// Consumer side, one thread only. Returns the oldest published signal.
/// `None` means empty, or that the oldest claimed slot is still 0 because
/// its producer has not stored yet: come back after that producer's
Expand Down
61 changes: 60 additions & 1 deletion test/js/web/web-globals.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { spawn } from "bun";
import { expect, it, test } from "bun:test";
import { describe, expect, it, test } from "bun:test";
import { bunEnv, bunExe, isLinux, isMacOS, isWindows, tempDir, withoutAggressiveGC } from "harness";

test("exists", () => {
Expand Down Expand Up @@ -419,6 +419,65 @@ test("confirm (no) windows newline", async () => {
expect(await proc.stderr.text()).toBe("No\n");
});

// prompt(), confirm() and alert() block the JS thread on stdin. A JS signal
// listener must still run when the signal arrives, not after the next line.
describe.skipIf(isWindows)("dialogs run JS signal listeners while they wait for input", () => {
async function run(dialog, signal, code) {
const script = `
process.on(${JSON.stringify(signal)}, () => { console.error("[listener ran]"); process.exit(${code}); });
const answer = ${dialog};
console.error("[code after the dialog ran with " + JSON.stringify(answer) + "]");
`;
await using proc = spawn({ cmd: [bunExe(), "-e", script], stdio: ["pipe", "pipe", "pipe"], env: bunEnv });

// The dialog text is flushed right before the child waits on stdin.
const reader = proc.stdout.getReader();
let shown = "";
while (!shown.includes("? ")) {
const { value, done } = await reader.read();
if (done) break;
shown += Buffer.from(value).toString();
}
proc.kill(signal);
// Then answer the dialog. Without the fix the answer is consumed first and
// the code after the dialog runs. With it the child may already be gone,
// so a closed pipe (EPIPE) is the expected outcome, not a failure.
try {
proc.stdin.write("bob\n");
await proc.stdin.end();
} catch (e) {
if (e?.code !== "EPIPE") throw e;
}

const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
return { shown, stderr, exitCode };
}

test.concurrent("prompt() and SIGINT", async () => {
expect(await run('prompt("name?")', "SIGINT", 130)).toEqual({
shown: "name? ",
stderr: "[listener ran]\n",
exitCode: 130,
});
});

test.concurrent("confirm() and SIGTERM", async () => {
expect(await run('confirm("sure?")', "SIGTERM", 143)).toEqual({
shown: "sure? [y/N] ",
stderr: "[listener ran]\n",
exitCode: 143,
});
});

test.concurrent("alert() and SIGINT", async () => {
expect(await run('alert("done?")', "SIGINT", 130)).toEqual({
shown: "done? [Enter] ",
stderr: "[listener ran]\n",
exitCode: 130,
});
});
});
Comment thread
robobun marked this conversation as resolved.
Resolved

test("globalThis.self = 123 works", () => {
expect(Object.getOwnPropertyDescriptor(globalThis, "self")).toMatchObject({
configurable: true,
Expand Down
Loading