tokio: Add back poll_* for udp #2981

leshow · 2020-10-17T04:07:00Z

As discussed in #2830 the first step to getting UdpFramed back is the poll functions. I've added these back, following the new style.

I could have just re-enabled UdpFramed on this PR too, but I thought we might take the opportunity to discuss any changes that we may want to make to that. In it's current state it still requires a split-like API to do concurrent send/recv

Darksonn

These shouldn't be #[doc(hidden)].

leshow · 2020-10-17T17:11:14Z

Cool, I had them as hidden because that's what they used to be, I'll write up some docs for the methods.

The only async method that doesn't have a poll_ version right now is connect. Do we need one?

tokio/src/net/udp/socket.rs

95th · 2020-10-19T06:57:17Z

tokio/src/net/udp/socket.rs

+    /// This function may encounter any standard I/O error except `WouldBlock`.
+    ///
+    /// [`connect`]: method@Self::connect
+    pub fn poll_recv(&self, cx: &mut Context<'_>, buf: &mut [u8]) -> Poll<io::Result<usize>> {


Shouldn't buf be the new ReadBuf here?

Good point. Same for poll_recv_from.

Ah okay. For converting ReadBuf -> &mut buf there's a few methods that we could use. initialize_unfilled which is safe and initializes the buffer, and unfilled_mut which is unsafe and assumes it's initialized. Methods in TcpStream use the unsafe version but they are private, if that makes any difference.

edit: why exactly do we want ReadBuf exposed here? UdpSocket doesn't even implement AsyncRead which is where ReadBuf is mainly used/exposed

Well ReadBuf would allow receiving into uninitialized memory. I'm pretty sure the underlying IO primitives should support that. Even if you prefer to write a version that initializes it first, defining it with ReadBuf would be necessary for adding that optimization later without a breaking change.

Alright, updated.

seanmonstar

My only thought is that we should document that these poll_ methods have a downside of only being able to be associated to one task per read/write direction, as opposed to the async fn methods, which should be preferred if you don't require polling specifically.

leshow · 2020-10-19T21:30:30Z

Right, I remember reading this about TcpStream, I can go back and add that. Does that limitation decrease their use in a possible UdpFramed implementation?

seanmonstar · 2020-10-19T22:00:14Z

I don't think it decreases the use there. That's a case where Stream is using the read direction, and Sink the write direction, and you can't use async fn, so the poll functions are correct.

Darksonn

Using the phrasing from Future.

tokio/src/net/udp/socket.rs

leshow · 2020-10-20T23:41:05Z

I was poking around in mio::UdpSocket and noticed it has functions for peek_from that aren't exposed, any problems with adding that to this PR? What about poll_peek?

Darksonn · 2020-10-21T06:17:16Z

We already have those on TcpStream, so it should be fine. Remember ReadBuf on poll_peek.

tokio/src/net/udp/socket.rs

Darksonn · 2020-10-21T11:36:42Z

tokio/src/net/udp/socket.rs

+    pub fn poll_recv(
+        &self,
+        cx: &mut Context<'_>,
+        buf: &mut ReadBuf<'_>,
+    ) -> Poll<io::Result<usize>> {


Since it uses ReadBuf, it should return Result<()> for consistency with AsyncReadExt::read.

I was going to reply saying poll_peek on TcpStream returns the size, but I just noticed it also does not use ReadBuf. That's something that's been overlooked, yes? I can add a quick fix there also.

edit: It appears the use of &mut [u8] in TcpStream poll_ is pervasive and may be deserving of it's own PR. Open to suggestions

Yes, TcpStream::poll_peek was overlooked. See #2987.

tokio/src/net/udp/socket.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Darksonn

I think this is ready besides a doc typo.

tokio/src/net/udp/socket.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

leshow force-pushed the add_udp_poll branch from 5f35b00 to 826107b Compare October 17, 2020 04:07

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net labels Oct 17, 2020

Darksonn requested a review from carllerche October 17, 2020 10:07

Darksonn reviewed Oct 17, 2020

View reviewed changes

leshow force-pushed the add_udp_poll branch from 826107b to 8c45738 Compare October 17, 2020 17:22

leshow commented Oct 17, 2020

View reviewed changes

tokio/src/net/udp/socket.rs Show resolved Hide resolved

leshow force-pushed the add_udp_poll branch 2 times, most recently from e803ced to b260e0c Compare October 17, 2020 17:28

carllerche requested a review from seanmonstar October 17, 2020 17:48

95th reviewed Oct 19, 2020

View reviewed changes

leshow force-pushed the add_udp_poll branch from 00a16fc to b260e0c Compare October 19, 2020 14:38

Darksonn mentioned this pull request Oct 19, 2020

Change recv and recv_from to use ReadBuf on UdpSocket and UnixDatagram #2988

Closed

seanmonstar reviewed Oct 19, 2020

View reviewed changes

leshow force-pushed the add_udp_poll branch 2 times, most recently from 8418437 to b2aef3e Compare October 19, 2020 22:28

Darksonn reviewed Oct 20, 2020

View reviewed changes

tokio/src/net/udp/socket.rs Outdated Show resolved Hide resolved

leshow force-pushed the add_udp_poll branch from b2aef3e to 990c40f Compare October 20, 2020 15:54

Darksonn reviewed Oct 21, 2020

View reviewed changes

leshow and others added 3 commits October 21, 2020 17:00

tokio: Add back poll_* for udp

9363fa8

Add ReadBuf to UdpSocket poll_recv

fbdc27f

udp: minor formatting changes (code review)

599986e

Co-authored-by: Alice Ryhl <alice@ryhl.io>

leshow force-pushed the add_udp_poll branch from c28cfbe to 3871034 Compare October 21, 2020 21:00

udp: add peek_from & poll_peek_from

4a959df

leshow force-pushed the add_udp_poll branch from 3871034 to 4a959df Compare October 21, 2020 21:15

leshow mentioned this pull request Oct 22, 2020

The tokio::net::udp module is empty #2989

Closed

Darksonn approved these changes Oct 22, 2020

View reviewed changes

tokio/src/net/udp/socket.rs Outdated Show resolved Hide resolved

udp: docs fix

57e9306

Co-authored-by: Alice Ryhl <alice@ryhl.io>

carllerche merged commit 358e4f9 into tokio-rs:master Oct 22, 2020

Darksonn mentioned this pull request Dec 7, 2020

net: expose poll_* methods on UnixDatagram #3223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tokio: Add back poll_* for udp #2981

tokio: Add back poll_* for udp #2981

leshow commented Oct 17, 2020

Darksonn left a comment

leshow commented Oct 17, 2020 •

edited

95th Oct 19, 2020

Darksonn Oct 19, 2020

leshow Oct 19, 2020 •

edited

Darksonn Oct 19, 2020

leshow Oct 19, 2020

seanmonstar left a comment

leshow commented Oct 19, 2020

seanmonstar commented Oct 19, 2020

Darksonn left a comment

leshow commented Oct 20, 2020 •

edited

Darksonn commented Oct 21, 2020

Darksonn Oct 21, 2020

leshow Oct 21, 2020 •

edited

Darksonn Oct 21, 2020

Darksonn left a comment

tokio: Add back poll_* for udp #2981

tokio: Add back poll_* for udp #2981

Conversation

leshow commented Oct 17, 2020

Darksonn left a comment

Choose a reason for hiding this comment

leshow commented Oct 17, 2020 • edited

95th Oct 19, 2020

Choose a reason for hiding this comment

Darksonn Oct 19, 2020

Choose a reason for hiding this comment

leshow Oct 19, 2020 • edited

Choose a reason for hiding this comment

Darksonn Oct 19, 2020

Choose a reason for hiding this comment

leshow Oct 19, 2020

Choose a reason for hiding this comment

seanmonstar left a comment

Choose a reason for hiding this comment

leshow commented Oct 19, 2020

seanmonstar commented Oct 19, 2020

Darksonn left a comment

Choose a reason for hiding this comment

leshow commented Oct 20, 2020 • edited

Darksonn commented Oct 21, 2020

Darksonn Oct 21, 2020

Choose a reason for hiding this comment

leshow Oct 21, 2020 • edited

Choose a reason for hiding this comment

Darksonn Oct 21, 2020

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

leshow commented Oct 17, 2020 •

edited

leshow Oct 19, 2020 •

edited

leshow commented Oct 20, 2020 •

edited

leshow Oct 21, 2020 •

edited