-
Notifications
You must be signed in to change notification settings - Fork 229
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.
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
impl AsyncReadRentAt and AsyncWriteRentAt for File #309
Conversation
monoio/src/io/async_read_rent.rs
Outdated
@@ -43,7 +43,7 @@ pub trait AsyncReadRent { | |||
pub trait AsyncReadRentAt { | |||
/// Same as pread(2) | |||
fn read_at<T: IoBufMut>( | |||
&mut self, | |||
&self, |
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.
It is useful to keep &mut self
, refer to #299 (comment)
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.
It's not clear to me that AsyncReadRent
and AsyncReadRentAt
need to align their receiver types. I'm pretty sure all platforms that monoio targets can implement a non-mut
read_at/write_at?
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.
Not only targets IO but also middlewares needs to implement AsyncReadRentAt, such as if we want to implement AsyncReadRentAt for https://doc.rust-lang.org/src/std/io/buffered/bufreader.rs.html#317-329 -like buffer reader, it needs inner mutations (fill buf)
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.
I agree with @ethe, there may be state(like buffer) inside the middlewares that implements AsyncReadRent. So for the struct itself, we can expose &self
if possible; and for the trait we can keep &mut self
.
Usually this style works well for stream(like tcp/uds) and file io, because even we expose &self
, in most cases users want to make sure something like write_all
not run in parallel(otherwise multiple write
may results in data out of order).
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.
AsyncReadRentAt
should only be implemented by types that support an efficient positional read operation, so shouldn't buffering occurring at a different level, e.g. via a wrapper?
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.
The reason I'm pushing back a bit here is b/c a lot of things will sit downstream of this foundational trait. I'm currently exploring using it for writing a file format client, and needing to Mutex
-wrap or .clone()
file handles to allow parallel access for reading feels wasteful.
It's common to have a set of traits in your own library, and provide blanket-impls for something like the tokio or monoio IO traits to allow interop. This is what fusio does, this is also something we've done in Vortex.
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.
If we use buffered reader as a specific type, we would loose the polymorphism, we could not implement AsyncReadRentAt
for buffered reader any more, I am a little bit worry about this. Considering the tradition: Cursor
and BufReader
in std, impl Read for WrapperReader
allows user define a recursive impl Read
.
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.
I think cloning fd takes few cost, what do you think about it @a10y ? I tend to keep file not syncable.
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.
Yea I can go ahead and update the PR to bring back the mutable receiver
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.
Thank you!
Fixes #308