feat: implement readable and writable for Tcp Split - #388
Conversation
|
|
|
Hello, there are some clippy errors, could you help please to fix them, thank you very much. |
There was a problem hiding this comment.
Pull request overview
This PR exposes stream readiness waiting on TcpStream’s owned split halves so users can await read/write readiness via TcpOwnedReadHalf and TcpOwnedWriteHalf.
Changes:
- Add
TcpOwnedReadHalf::readable(relaxed)as a proxy toTcpStream::readable(relaxed). - Add
TcpOwnedWriteHalf::writable(relaxed)as a proxy toTcpStream::writable(relaxed). - Introduce accompanying Rustdoc describing readiness semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Note: Do not use it before every io. It is different from other runtimes! | ||
| /// | ||
| /// Everytime call to this method may pay a syscall cost. | ||
| /// In uring impl, it will push a PollAdd op; in epoll impl, it will use use | ||
| /// inner readiness state; if !relaxed, it will call syscall poll after that. |
There was a problem hiding this comment.
Doc comment typos: "Everytime" should be "Every time", and "use use" contains a duplicated word. Please fix these in the new docs to avoid propagating the typo further.
| /// If you want to do io by your own, you must maintain io readiness and wait | ||
| /// for io ready with relaxed=false. | ||
| #[inline] | ||
| pub async fn writable(&self, relaxed: bool) -> Result<(), io::Error> { |
There was a problem hiding this comment.
The new writable API returns Result<(), io::Error> while other socket readiness APIs use io::Result<()>. Please align this signature to io::Result<()> for consistency.
| pub async fn writable(&self, relaxed: bool) -> Result<(), io::Error> { | |
| pub async fn writable(&self, relaxed: bool) -> io::Result<()> { |
| /// Note: Do not use it before every io. It is different from other runtimes! | ||
| /// | ||
| /// Everytime call to this method may pay a syscall cost. | ||
| /// In uring impl, it will push a PollAdd op; in epoll impl, it will use use | ||
| /// inner readiness state; if !relaxed, it will call syscall poll after that. |
There was a problem hiding this comment.
Doc comment typos duplicated from the readable docs (e.g., "Everytime" and "use use"). Since this is newly added documentation, please correct the wording here as well.
| pub async fn readable(&self, relaxed: bool) -> Result<(), io::Error> { | ||
| unsafe { &*self.0.get() }.readable(relaxed).await | ||
| } |
There was a problem hiding this comment.
New public APIs (TcpOwnedReadHalf::readable / TcpOwnedWriteHalf::writable) aren’t covered by tests yet. There are existing readiness tests for TcpStream/TcpListener (e.g. monoio/tests/tcp_echo.rs::rw_able), so adding a similar test for the split halves would help prevent regressions and ensure these proxies work across drivers.
| pub async fn readable(&self, relaxed: bool) -> Result<(), io::Error> { | ||
| unsafe { &*self.0.get() }.readable(relaxed).await | ||
| } |
There was a problem hiding this comment.
The new readable API returns Result<(), io::Error> while the rest of this module (and TcpStream::{readable,writable}) use io::Result<()>. Please switch to io::Result<()> here for consistency and simpler signatures.
Hi @Lzzzzzt. The clippy errors do not stem from my commit and fixing them would pollute this PR. Should I still include the fix here? |
Problem
TcpOwnedReadHalfandTcpOwnedWriteHalfcurrently do not support waiting for the stream to be readable or writable, as the split does not expose the underlyingreadableandwritablefunctions.Design
We can safely expose both functions on the Read and Write split of the TcpStream since this does not require manipulation of a shared Fd and merely exposes already-implemented functions.
Solution
Adds proxy functions to
TcpStream'swritable(relaxed)andreadable(relaxed)functions forTcpOwnedWriteHalfandTcpOwnedReadHalf.