Skip to content
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

net: Add try_read_buf and try_recv_buf #3351

Merged
merged 7 commits into from
Jan 2, 2021
Merged

Conversation

cssivision
Copy link
Contributor

@cssivision cssivision commented Dec 26, 2020

Refs: #3313

two questions:

  • what i have done was in the right direction?
  • what a vectored read function should look like?
    function signature like below?
pub fn try_read_vectored<B: BufMut>(&self, buf: &mut [B]) -> io::Result<usize>;

@Darksonn
Copy link
Contributor

Yeah, this is the right direction. As for vectored reads, I realized that this is mainly relevant for writes rather than reads, so you don't need to do that. (The way you do it for writes is call chunks_vectored inside the write buf method.)

buf: &'a mut B,
) -> io::Result<usize>
where
&'a E: io::Read + 'a,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this lifetime is necessary.

Suggested change
&'a E: io::Read + 'a,
&'a E: io::Read,

Comment on lines 183 to 188
let dst = buf.chunk_mut();
let dst = &mut *(dst as *mut _ as *mut [std::mem::MaybeUninit<u8>]);
let mut buf = ReadBuf::uninit(dst);

let b = &mut *(buf.unfilled_mut() as *mut [std::mem::MaybeUninit<u8>] as *mut [u8]);
let n = self.io.as_ref().unwrap().read(b)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to go through ReadBuf here.

Suggested change
let dst = buf.chunk_mut();
let dst = &mut *(dst as *mut _ as *mut [std::mem::MaybeUninit<u8>]);
let mut buf = ReadBuf::uninit(dst);
let b = &mut *(buf.unfilled_mut() as *mut [std::mem::MaybeUninit<u8>] as *mut [u8]);
let n = self.io.as_ref().unwrap().read(b)?;
let dst = buf.chunk_mut();
let dst = &mut *(dst as *mut _ as *mut [std::mem::MaybeUninit<u8>] as *mut [u8]);
let n = self.io.as_ref().unwrap().read(dst)?;

Comment on lines 690 to 693
pub fn try_recv_buf<B: BufMut>(&self, buf: &mut B) -> io::Result<usize> {
self.io
.registration()
.try_io(Interest::READABLE, || unsafe { self.io.read_buf(buf) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need to do something different for the recv_buf variants. I'm not sure there is an equivalent trait to Read for datagram types.

Comment on lines 564 to 565
/// Try to read data from the stream into the provided buffer, advancing the
/// buffer's internal cursor, returning how many bytes were read.
Copy link
Contributor

@Darksonn Darksonn Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation could use an example. You can take inspiration from the existing try_read methods, but reading into a vector created using Vec::with_capacity(1024) to show how you can read into the unused capacity of a vector.

@cssivision cssivision changed the title [WIP] net: Add try_read_buf and try_recv_buf net: Add try_read_buf and try_recv_buf Dec 27, 2020
@cssivision
Copy link
Contributor Author

not sure whether or not it's a good idea to put this in io-util feature.

tokio/src/net/tcp/stream.rs Outdated Show resolved Hide resolved
tokio/src/net/unix/datagram/socket.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem reasonable to me.

tokio/tests/uds_stream.rs Show resolved Hide resolved
@taiki-e taiki-e added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jan 2, 2021
@Darksonn Darksonn merged commit 3b6bee8 into tokio-rs:master Jan 2, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Jan 2, 2021

Thank you for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants