Skip to content

Commit

Permalink
Fix read_generic blocking on non-blocking files when a length is not …
Browse files Browse the repository at this point in the history
…specified (#117)

* Fix read_generic blocking on non-blocking files when a length is not specified

* Cargo fmt
  • Loading branch information
twizmwazin authored Feb 23, 2024
1 parent 2a63e8b commit 2818c3a
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 11 deletions.
2 changes: 1 addition & 1 deletion crates/bh_agent_server/src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ mod unix_functions;
pub use read_chars::*;
pub use read_lines::read_lines;
#[cfg(target_family = "unix")]
pub use set_blocking::set_blocking;
pub use set_blocking::{is_blocking, set_blocking};
#[cfg(target_family = "unix")]
pub use unix_functions::{chmod, chown, stat};
48 changes: 39 additions & 9 deletions crates/bh_agent_server/src/util/read_chars.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,47 @@ use unicode_reader::Graphemes;

use bh_agent_common::FileOpenType;

pub fn read_generic(file: &mut File, n: Option<u32>, file_type: FileOpenType) -> Result<Vec<u8>> {
#[cfg(target_family = "unix")]
use crate::util::is_blocking;

// This is uhhh... a bit messy. The error handling is the main issue that is
//necessitating this inner function.
pub fn read_generic(
file: &mut File,
mut n: Option<u32>,
file_type: FileOpenType,
) -> Result<Vec<u8>> {
trace!("Entering read_generic");
if let Some(num_bytes) = n {
match file_type {
FileOpenType::Binary => Ok(read_bytes(file, num_bytes as usize)?),
FileOpenType::Text => Ok(read_graphemes(file, num_bytes as usize)?),
}
} else {
// if n is None, we just read the whole file, text parsing happens on the client
Ok(file.bytes().collect::<Result<Vec<u8>, std::io::Error>>()?)

#[cfg(target_family = "unix")]
if n.is_none() && !is_blocking(file)? {
n = Some(1024_u32.pow(2));
}

let inner: Result<Vec<u8>> = (|| {
trace!("read_generic: entering inner closure...");
if let Some(num_bytes) = n {
match file_type {
FileOpenType::Binary => Ok(read_bytes(file, num_bytes as usize)?),
FileOpenType::Text => Ok(read_graphemes(file, num_bytes as usize)?),
}
} else {
// if n is None, we just read the whole file, text parsing happens on the client
Ok(file.bytes().collect::<Result<Vec<u8>, std::io::Error>>()?)
}
})();
trace!("read_generic: processing inner result...");
inner.or_else(|e| {
if let Some(std::io::ErrorKind::WouldBlock) =
e.downcast_ref::<std::io::Error>().map(|e| e.kind())
{
trace!("read_generic: WouldBlock error, returning empty vec");
Ok(Vec::new())
} else {
trace!("read_generic: returning error");
Err(e)
}
})
}

fn read_bytes(file: &mut File, n: usize) -> Result<Vec<u8>> {
Expand Down
12 changes: 11 additions & 1 deletion crates/bh_agent_server/src/util/set_blocking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::io;
use std::os::unix::io::AsRawFd;

// TODO: This is using libc directly, but nix has a wrapper for this. We should use that instead.
pub fn set_blocking<T: AsRawFd>(fd: &T, blocking: bool) -> io::Result<()> {
pub fn set_blocking(fd: &impl AsRawFd, blocking: bool) -> io::Result<()> {
let raw_fd = fd.as_raw_fd();
let flags = unsafe { fcntl(raw_fd, F_GETFL, 0) };
if flags < 0 {
Expand All @@ -22,3 +22,13 @@ pub fn set_blocking<T: AsRawFd>(fd: &T, blocking: bool) -> io::Result<()> {

Ok(())
}

pub fn is_blocking(fd: &impl AsRawFd) -> io::Result<bool> {
let raw_fd = fd.as_raw_fd();
let flags = unsafe { fcntl(raw_fd, F_GETFL, 0) };
if flags < 0 {
return Err(io::Error::last_os_error());
}

Ok(flags & O_NONBLOCK == 0)
}

0 comments on commit 2818c3a

Please sign in to comment.