impl AsyncReadRentAt and AsyncWriteRentAt for File #309
Conversation
|
|
a10y
changed the title
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.
It is useful to keep &mut self, refer to #299 (comment)
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
Yea I can go ahead and update the PR to bring back the mutable receiver
Fixes #308