Repository navigation
process.stdout/stderr: clear O_NONBLOCK for spawned children, don't drop console output on EAGAIN #43868
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,26 +61,57 @@ pub trait PosixPipeWriter { | |
|
|
||
| fn handle(&self) -> &PollOrFd; | ||
|
|
||
| /// Set once `pwritev2(RWF_NOWAIT)` said this fd's file type does not support it, so we stop asking. | ||
| fn rwf_unsupported(&self) -> &core::cell::Cell<bool>; | ||
|
|
||
| /// Only reads `get_file_type()` / `get_fd()` from `self`; takes `&self` so | ||
| /// callers may pass a `buf` that borrows from a field of `self` (e.g. | ||
| /// `self.outgoing.slice()`) without raw-pointer aliasing escapes. | ||
| fn try_write(&self, force_sync: bool, buf: &[u8]) -> WriteResult { | ||
| // PERF: try_write_with_write_fn is not monomorphized per FileType — | ||
| // profile if hot. | ||
| let ft = self.get_file_type(); | ||
| // Linux: RWF_NOWAIT is a free per-call nonblocking write, so a pipe stays async even after a spawn cleared O_NONBLOCK on it. | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| if matches!(ft, FileType::Pipe | FileType::NonblockingPipe) && !self.rwf_unsupported().get() | ||
| { | ||
| match self.try_write_nowait(buf) { | ||
| Some(rc) => return rc, | ||
| // Not a pipe as far as pwritev2 is concerned (tty, pty master, old kernel): remember, use the plain path. | ||
| None => self.rwf_unsupported().set(true), | ||
| } | ||
| } | ||
| match ft { | ||
| // send(MSG_DONTWAIT | MSG_NBIO) is nonblocking per call on every Unix, whatever the fd's flags. | ||
| FileType::Socket => self.try_write_with_write_fn(buf, write_to_socket), | ||
| FileType::Pipe if !force_sync => { | ||
| self.try_write_with_write_fn(buf, write_to_blocking_pipe) | ||
| } | ||
| _ => self.try_write_with_write_fn(buf, sys::write), | ||
| } | ||
| } | ||
|
|
||
| /// `None` if `RWF_NOWAIT` is not usable on this fd (nothing was written). | ||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| fn try_write_nowait(&self, buf: &[u8]) -> Option<WriteResult> { | ||
| let fd = self.get_fd(); | ||
| if fd == Fd::INVALID { | ||
| return Some(WriteResult::Done(0)); | ||
| } | ||
| let mut offset: usize = 0; | ||
| while offset < buf.len() { | ||
| match sys::write_nowait(fd, &buf[offset..]) { | ||
| Ok(None) if offset == 0 => return None, | ||
| Ok(None) => return Some(WriteResult::Pending(offset)), | ||
| Ok(Some(0)) => return Some(WriteResult::Done(offset)), | ||
| Ok(Some(wrote)) => offset += wrote, | ||
| Err(err) if err.is_retry() => return Some(WriteResult::Pending(offset)), | ||
| Err(err) => return Some(WriteResult::Err(err)), | ||
| } | ||
| } | ||
| Some(WriteResult::Wrote(offset)) | ||
| } | ||
|
|
||
| fn try_write_with_write_fn( | ||
| &self, | ||
| buf: &[u8], | ||
|
|
@@ -219,13 +250,6 @@ pub trait PosixPipeWriter { | |
| /// Free fn for the blocking-pipe path; the other file types are handled | ||
| /// inline in `try_write` above. | ||
| fn write_to_blocking_pipe(fd: Fd, buf: &[u8]) -> sys::Result<usize> { | ||
| match bun_core::is_writable(fd) { | ||
| bun_core::Pollable::Ready | bun_core::Pollable::Hup => sys::write(fd, buf), | ||
| bun_core::Pollable::NotReady => sys::Result::Err(sys::Error::retry()), | ||
|
|
@@ -237,6 +261,12 @@ fn write_to_blocking_pipe(fd: Fd, buf: &[u8]) -> sys::Result<usize> { | |
| fn write_to_socket(fd: Fd, buf: &[u8]) -> sys::Result<usize> { | ||
| sys::send_non_block(fd, buf).map_err(|err| sys::Error { | ||
| syscall: sys::Tag::write, | ||
| // XNU's send() says ENOTCONN once a stream peer is fully gone; write(2) on the same fd (and Node) say EPIPE. | ||
| errno: if err.get_errno() == sys::E::ENOTCONN { | ||
|
Comment on lines
+264
to
+265
Contributor
There was a problem hiding this comment. 🟡 nit (optional): the new ENOTCONN-to-EPIPE remap in write_to_socket ships with no assertion that observes it, so a future regression back to ENOTCONN passes the suite. The only test that hits this path, test/js/bun/console/console-write.test.ts:134-136, still accepts either code on darwin, an allowance this change makes dead. Fix: tighten that test to expect exactly "caught EPIPE\n" on every platform (or add a sibling case), so the remap at src/io/PipeWriter.rs:265 is covered and the stale darwin branch goes away. Why this was flaggedA macOS user runs a child whose stdout is a Bun.spawn socketpair and the parent cancels its reader mid-write; the child's console.write / process.stdout.write reaches write_to_socket (src/io/PipeWriter.rs:262) and XNU's send() returns ENOTCONN. The diff at src/io/PipeWriter.rs:264-269 now rewrites that errno to EPIPE before it reaches JS, which is the intended Node-matching behaviour and is not exercised by any of the nine new tests in test/js/node/process/process-stdio.test.ts. The pre-existing test test/js/bun/console/console-write.test.ts:106-138 reproduces exactly this scenario, but line 135 keeps Verification: nit — triggers whenever the ENOTCONN→EPIPE remap regresses (or is dropped) on macOS: no test in the PR would fail. The diff adds at src/io/PipeWriter.rs:264-269
Collaborator
There was a problem hiding this comment. This comes from the rebase. #44072 merged into Two facts from
I changed nothing here. The request was a rebase, and I cannot run tests on macOS. There are two ways to make the branch consistent:
Tell me which one you want and I will push it. |
||
| sys::E::EPIPE as _ | ||
| } else { | ||
| err.errno | ||
| }, | ||
| ..err | ||
| }) | ||
| } | ||
|
|
@@ -285,6 +315,7 @@ pub struct PosixBufferedWriter<Parent: PosixBufferedWriterParent> { | |
| pub(crate) pollable: bool, | ||
| pub(crate) closed_without_reporting: bool, | ||
| pub close_fd: bool, | ||
| rwf_unsupported: core::cell::Cell<bool>, | ||
| } | ||
|
|
||
| impl<Parent: PosixBufferedWriterParent> Default for PosixBufferedWriter<Parent> { | ||
|
|
@@ -296,6 +327,7 @@ impl<Parent: PosixBufferedWriterParent> Default for PosixBufferedWriter<Parent> | |
| pollable: false, | ||
| closed_without_reporting: false, | ||
| close_fd: true, | ||
| rwf_unsupported: core::cell::Cell::new(false), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -325,6 +357,9 @@ impl<Parent: PosixBufferedWriterParent> PosixPipeWriter for PosixBufferedWriter< | |
| fn handle(&self) -> &PollOrFd { | ||
| &self.handle | ||
| } | ||
| fn rwf_unsupported(&self) -> &core::cell::Cell<bool> { | ||
| &self.rwf_unsupported | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: writer is an intrusive field of `Parent`; `Parent::on_write` | ||
|
|
@@ -610,6 +645,7 @@ pub struct PosixStreamingWriter<Parent: PosixStreamingWriterParent> { | |
| pub force_sync: bool, | ||
| /// Last reported `WriteStatus == Pending` (i.e. write(2) returned EAGAIN). | ||
| backed_up: core::cell::Cell<bool>, | ||
| rwf_unsupported: core::cell::Cell<bool>, | ||
| } | ||
|
|
||
| impl<Parent: PosixStreamingWriterParent> Default for PosixStreamingWriter<Parent> { | ||
|
|
@@ -622,6 +658,7 @@ impl<Parent: PosixStreamingWriterParent> Default for PosixStreamingWriter<Parent | |
| closed_without_reporting: false, | ||
| force_sync: false, | ||
| backed_up: core::cell::Cell::new(false), | ||
| rwf_unsupported: core::cell::Cell::new(false), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -651,6 +688,9 @@ impl<Parent: PosixStreamingWriterParent> PosixPipeWriter for PosixStreamingWrite | |
| fn handle(&self) -> &PollOrFd { | ||
| &self.handle | ||
| } | ||
| fn rwf_unsupported(&self) -> &core::cell::Cell<bool> { | ||
| &self.rwf_unsupported | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: see `PosixBufferedWriter`'s `LaunderedSelf` impl — identical shape. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit (optional): maintainers now have the private XNU MSG_NBIO value written twice as a bare literal, so a future correction to one copy silently misses the other. bsd.c:66 spells it as
0x20000inside LIBUS_SEND_DONTWAIT while src/sys/lib.rs:3104 definesMSG_NBIO: i32 = 0x20000for the same purpose. Fix: give the C side a namedMSG_NBIO-style define (with the XNU source line it comes from) that LIBUS_SEND_DONTWAIT ORs in, so both send paths (bsd_send, bsd_sendmsg) and the RustSEND_FLAGS_NONBLOCKname the same constant instead of repeating the magic number.Why this was flagged
The diff adds the Darwin-private send flag in two unrelated modules: packages/bun-usockets/src/bsd.c:66
#define LIBUS_SEND_DONTWAIT (MSG_DONTWAIT | 0x20000)and src/sys/lib.rs:3104const MSG_NBIO: i32 = 0x20000;. Neither references the other, and the C copy is an unnamed literal inside a macro. If the value or its applicability is ever revised (for example after the private-flag concern already discussed on this PR), whoever edits src/sys/lib.rs will not find the bsd.c copy by name, and usockets TCP/unix socket sends on macOS keep the stale flag while FileSink/spawn stdio sends use the new one. No runtime failure today; the base branch had neither copy. Repository review guidance asks that a fact live in one place and that magic numbers be named constants.Verification: nit. Both citations check out: packages/bun-usockets/src/bsd.c:66 adds
#define LIBUS_SEND_DONTWAIT (MSG_DONTWAIT | 0x20000)with the value as an anonymous literal inside the macro (only the comment on line 65 names it as MSG_NBIO), and src/sys/lib.rs:3104 addsconst MSG_NBIO: i32 = 0x20000;(macOS-gated, folded intoSEND_FLAGS_NONBLOCKat :3108). The two definitions are in separate modules…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving the literal in both places: the C side shares no header with bun_sys, and each site carries the one-line XNU note.