fix(oxfmt): handle WouldBlock when writing to stdout#17943
fix(oxfmt): handle WouldBlock when writing to stdout#17943
WouldBlock when writing to stdout#17943Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #17939 by improving the error handling in the print_and_flush function to properly handle WouldBlock errors when writing to stdout. The implementation adds a retry mechanism with thread yielding to handle cases where the output buffer is temporarily full.
Changes:
- Replaced simple
write_allwith a manual write loop that handles partial writes andWouldBlockerrors - Added retry logic with a maximum retry count for both write and flush operations
- Improved handling of
InterruptedandBrokenPipeerror conditions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(error) | ||
| use std::io::ErrorKind; | ||
|
|
||
| const MAX_RETRIES: u32 = 1000; |
There was a problem hiding this comment.
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.
| @@ -39,16 +39,47 @@ | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
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.
| /// 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. |
apps/oxfmt/src/core/utils.rs
Outdated
| panic!("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; | ||
| if retries >= MAX_RETRIES { | ||
| panic!("Failed to flush: too many WouldBlock retries"); |
There was a problem hiding this comment.
The panic messages "Failed to write: too many WouldBlock retries" and "Failed to flush: too many WouldBlock retries" could be more helpful by including the actual number of retries attempted (MAX_RETRIES) and possibly suggesting what might be wrong (e.g., "Output buffer appears to be stuck"). This would help users understand why the operation failed.
| 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; | ||
|
|
||
| let mut bytes = message.as_bytes(); | ||
| let mut retries = 0; | ||
| while !bytes.is_empty() { | ||
| match writer.write(bytes) { | ||
| Ok(0) => break, // EOF | ||
| Ok(n) => { | ||
| bytes = &bytes[n..]; | ||
| retries = 0; // Reset on progress | ||
| } | ||
| Err(e) if e.kind() == ErrorKind::WouldBlock => { | ||
| retries += 1; | ||
| if retries >= MAX_RETRIES { | ||
| panic!("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; | ||
| if retries >= MAX_RETRIES { | ||
| panic!("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}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
There's no test coverage for the new WouldBlock handling logic. Since other modules in this codebase have comprehensive test coverage (oxfmtrc.rs, support.rs, options.rs, server_formatter.rs), consider adding tests to verify the retry behavior works correctly for WouldBlock scenarios, the retry limit is enforced, and that Interrupted errors are properly retried without counting against the limit.
| let mut retries = 0; | ||
| while !bytes.is_empty() { | ||
| match writer.write(bytes) { | ||
| Ok(0) => break, // EOF |
There was a problem hiding this comment.
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.
| Ok(0) => break, // EOF | |
| Ok(0) => panic!( | |
| "Failed to write: writer returned Ok(0) with {} bytes remaining", | |
| bytes.len() | |
| ), |
e504297 to
754664e
Compare
|
Thanks for your help! But I decided to handle it on #17950. |

fixes #17939