Skip to content

fix(oxfmt): handle WouldBlock when writing to stdout #17943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
camc314 wants to merge 1 commit into from
+36 −9
Closed

fix(oxfmt): handle WouldBlock when writing to stdout #17943

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 36 additions & 9 deletions apps/oxfmt/src/core/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,43 @@ pub fn read_to_string(path: &Path) -> io::Result<String> {
}

Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The function lacks documentation explaining its behavior, especially the new WouldBlock handling logic. Consider adding a doc comment that describes what the function does, when it panics, and how it handles different error conditions including WouldBlock, Interrupted, and BrokenPipe.

Suggested change
/// Writes the full `message` to the given writer and then flushes it, with
/// special handling for non-blocking and signal-interrupted I/O.
///
/// The function repeatedly calls `write` until all bytes of `message` have
/// been written or an unrecoverable condition occurs, and then repeatedly
/// calls `flush` until it succeeds or an unrecoverable condition occurs.
///
/// # Error handling
///
/// * `ErrorKind::Interrupted` — the operation is retried immediately.
/// * `ErrorKind::WouldBlock` — the operation is retried up to an internal
/// limit (`MAX_RETRIES`), yielding the current thread between attempts.
/// If the limit is exceeded, the function panics.
/// * `ErrorKind::BrokenPipe` — writing or flushing stops and the function
/// returns without panicking.
/// * Any other I/O error — the function panics. For write errors, the panic
/// message includes the number of bytes remaining to be written.
///
/// # Panics
///
/// This function panics if:
/// * writing or flushing encounters an I/O error other than
/// `Interrupted`, `WouldBlock`, or `BrokenPipe`, or
/// * a `WouldBlock` error occurs too many times in a row during either
/// writing or flushing.

Copilot uses AI. Check for mistakes.
pub fn print_and_flush(writer: &mut dyn Write, message: &str) {
use std::io::{Error, ErrorKind};
fn check_for_writer_error(error: Error) -> Result<(), Error> {
// Do not panic when the process is killed (e.g. piping into `less`).
if matches!(error.kind(), ErrorKind::Interrupted | ErrorKind::BrokenPipe) {
Ok(())
} else {
Err(error)
use std::io::ErrorKind;

const MAX_RETRIES: u32 = 1000;
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The MAX_RETRIES value of 1000 seems arbitrary and could lead to excessive CPU spinning in case of persistent WouldBlock errors. Consider adding a small sleep duration between retries (e.g., using std::thread::sleep with a short duration like 1ms) instead of just yield_now(), which may not be sufficient to allow the buffer to drain. This would be more resource-friendly and prevent busy-waiting.

Copilot uses AI. Check for mistakes.

let mut bytes = message.as_bytes();
let mut retries = 0;
while !bytes.is_empty() {
match writer.write(bytes) {
Ok(0) => break, // EOF
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

When write returns Ok(0), the loop breaks immediately, potentially leaving unwritten bytes in the buffer. According to std::io::Write documentation, returning Ok(0) typically indicates that the underlying object is no longer able to accept bytes, but it's not necessarily an EOF in the traditional sense for stdout. This could silently truncate output. Consider treating this as an error condition or at least adding a comment explaining why silent truncation is acceptable here.

Suggested change
Ok(0) => break, // EOF
Ok(0) => panic!(
"Failed to write: writer returned Ok(0) with {} bytes remaining",
bytes.len()
),

Copilot uses AI. Check for mistakes.
Ok(n) => {
bytes = &bytes[n..];
retries = 0; // Reset on progress
}
Err(e) if e.kind() == ErrorKind::WouldBlock => {
retries += 1;
assert!(retries < MAX_RETRIES, "Failed to write: too many WouldBlock retries");
// Buffer full, yield and retry
std::thread::yield_now();
}
Err(e) if e.kind() == ErrorKind::Interrupted => {}
Err(e) if e.kind() == ErrorKind::BrokenPipe => break,
Err(e) => panic!("Failed to write: {e} bytes: {}", bytes.len()),
}
}

writer.write_all(message.as_bytes()).or_else(check_for_writer_error).unwrap();
writer.flush().unwrap();
let mut retries = 0;
loop {
match writer.flush() {
Ok(()) => break,
Err(e) if e.kind() == ErrorKind::WouldBlock => {
retries += 1;
assert!(retries < MAX_RETRIES, "Failed to flush: too many WouldBlock retries");
std::thread::yield_now();
}
Err(e) if e.kind() == ErrorKind::Interrupted => {}
Err(e) if e.kind() == ErrorKind::BrokenPipe => break,
Err(e) => panic!("Failed to flush: {e}"),
}
}
}
Loading