util: UdpFramed takes Borrow<UdpSocket> #3451

leshow · 2021-01-20T20:51:18Z

Motivation

This changes UdpFramed from holding a UdpSocket to a Borrow<UdpSocket>.

Often, you will have an Arc<UdpSocket> which you want to share among many tasks but also still be able to use the helper functions from tokio-util. Unfortunately, UdpFramed only takes a UdpSocket.

~~The disadvantage of this approach is that into_inner and get_mut must be removed, does that seem like an acceptable trade-off for the added flexibility?~~

edit: into_inner doesn't actually have to be removed, ~~my mistake, only get_mut~~
edit: get_mut now implemented specifically for UdpFramed<C, UdpSocket>

Possible Solution?

Make UdpFramed take a T: Borrow<UdpSocket> so it can take UdpSocket types generically.

I'm happy to take any feedback or alternate approach that is suggested. But let me take your temperature first on if you think you'd like this change or not?

Darksonn · 2021-01-28T15:21:17Z

tokio-util/src/udp/frame.rs

-pub struct UdpFramed<C> {
-    socket: UdpSocket,
+pub struct UdpFramed<T, C> {
+    socket: T,


This is a breaking change, and would require a new tokio-util series.

Yeah, although I don't see any other way to enable this:

let a = Arc::new(UdpSocket::bind("127.0.0.1:0").await?); let b = Arc::clone(&a); let mut recv = UdpFramed::new(a, ByteCodec); let mut send = UdpFramed::new(b, ByteCodec); // possibly move recv/send to different tasks

I think it's a fairly common scenario that you'll have a UdpSocket in an Arc in order to share between different tasks? You may want to have one task doing the sending and one doing the recv'ing for example.

Would it help if it was a new type instead of changing UdpFramed?

edit: Just to make my case a little further. I the old api of taking just UdpSocket made more sense when tokio required &mut self to send/recv. Now that it's &self, I think this API makes more sense.

I think it would be possible to add it with a default as in UdpFramed<C, T = UdpSocket>.

Sure, I'll experiment with that and report back. Probably best to switch the type param order as you have there also.

That seemed to work. I was also able to add back get_mut by writing an impl block specifically for UdpFramed<C, T> where T: BorrowMut<UdpSocket> since BorrowMut is a parent trait to Borrow it will have access to all the other methods also.

Darksonn · 2021-03-27T20:50:45Z

You will probably need to merge or rebase on master to get the fixes for any warnings introduced to Rust after this PR was created.

leshow · 2021-03-28T18:30:09Z

Should be all up to date now.

Darksonn · 2021-04-12T12:27:25Z

tokio-util/src/udp/frame.rs

-impl<C: Decoder + Unpin> Stream for UdpFramed<C> {
+impl<C, T> Stream for UdpFramed<C, T>
+where
+    T: Borrow<UdpSocket> + Unpin,


You do not need the Unpin bound.

Suggested change

T: Borrow<UdpSocket> + Unpin,

T: Borrow<UdpSocket>,

You can even add an unconditional impl<C, T> Unpin for UdpFramed<C, T> {}

That last impl is what I was missing, without it the self.get_mut() line in Stream complains about Unpin. Thanks!

Darksonn · 2021-04-12T12:27:47Z

tokio-util/src/udp/frame.rs

@@ -79,7 +86,7 @@ impl<C: Decoder + Unpin> Stream for UdpFramed<C> {
                let buf = &mut *(pin.rd.chunk_mut() as *mut _ as *mut [MaybeUninit<u8>]);
                let mut read = ReadBuf::uninit(buf);
                let ptr = read.filled().as_ptr();
-                let res = ready!(Pin::new(&mut pin.socket).poll_recv_from(cx, &mut read));
+                let res = ready!(Pin::new(&mut pin.socket.borrow()).poll_recv_from(cx, &mut read));


Suggested change

let res = ready!(Pin::new(&mut pin.socket.borrow()).poll_recv_from(cx, &mut read));

let res = ready!(pin.socket.borrow().poll_recv_from(cx, &mut read));

Darksonn · 2021-04-12T12:28:27Z

tokio-util/src/udp/frame.rs

+    T: Borrow<UdpSocket> + Unpin,
+    C: Encoder<I> + Unpin,


Again, you don't need Unpin here.

Suggested change

T: Borrow<UdpSocket> + Unpin,

C: Encoder<I> + Unpin,

T: Borrow<UdpSocket>,

C: Encoder<I>,

I've added these and the above changes manually, I just want to call out that the previous bound was C: Encoder<I> + Unpin so we are removing an existing Unpin bound also, same with C: Decoder + Unpin

Okay, well, there should not have been any Unpin bounds in this file in the first place.

Darksonn · 2021-04-12T12:29:56Z

tokio-util/src/udp/frame.rs

-    /// with.
-    pub fn get_mut(&mut self) -> &mut UdpSocket {
-        &mut self.socket
-    }


If you remove get_mut, it becomes a breaking change. Maybe return &mut T here, then change get_ref to return &T for consistency.

Edit: I just noticed the get_mut impl further down. Not sure what I like best tbh.

I'm good with either, I used .borrow() and .borrow_mut() because I thought it may be a breaking change if it returned &T and a user updated, all of a sudden you have to call .borrow() additionally on that to get a &UdpSocket?

Well, any existing users would have T = UdpSocket, so if we return &T, that's an &UdpSocket, no?

The default param was a recent addition that changes things, good point. Okay, I've updated it to return the type params.

Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Jan 28, 2021

Darksonn reviewed Jan 28, 2021

View reviewed changes

leshow force-pushed the tokio_util_borrow branch 2 times, most recently from 82ce761 to 57ef66d Compare January 31, 2021 23:21

leshow force-pushed the tokio_util_borrow branch 2 times, most recently from 327064d to 669cece Compare March 27, 2021 20:31

leshow added 2 commits March 28, 2021 14:25

util: UdpFramed takes Borrow<UdpSocket>

b7bd9ef

add back into_inner

81dee86

leshow force-pushed the tokio_util_borrow branch from 669cece to 2470a8b Compare March 28, 2021 18:25

Darksonn reviewed Apr 12, 2021

View reviewed changes

leshow force-pushed the tokio_util_borrow branch 2 times, most recently from 0cf146b to d5b492c Compare April 13, 2021 14:48

use default param & add back get_mut

a5148bd

leshow force-pushed the tokio_util_borrow branch from d5b492c to a5148bd Compare April 13, 2021 15:32

Darksonn approved these changes Apr 14, 2021

View reviewed changes

Darksonn merged commit 9eeec03 into tokio-rs:master Apr 14, 2021

Darksonn mentioned this pull request May 14, 2021

chore: prepare tokio-util v0.6.7 #3784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: UdpFramed takes Borrow<UdpSocket> #3451

util: UdpFramed takes Borrow<UdpSocket> #3451

leshow commented Jan 20, 2021 •

edited

Loading

Darksonn Jan 28, 2021

leshow Jan 28, 2021 •

edited

Loading

Darksonn Mar 21, 2021

leshow Mar 22, 2021

leshow Mar 27, 2021 •

edited

Loading

Darksonn commented Mar 27, 2021

leshow commented Mar 28, 2021

Darksonn Apr 12, 2021

leshow Apr 12, 2021

Darksonn Apr 12, 2021

Darksonn Apr 12, 2021

leshow Apr 12, 2021 •

edited

Loading

Darksonn Apr 13, 2021

Darksonn Apr 12, 2021

leshow Apr 12, 2021 •

edited

Loading

Darksonn Apr 13, 2021

leshow Apr 13, 2021

	let res = ready!(Pin::new(&mut pin.socket.borrow()).poll_recv_from(cx, &mut read));
	let res = ready!(pin.socket.borrow().poll_recv_from(cx, &mut read));

util: UdpFramed takes Borrow<UdpSocket> #3451

util: UdpFramed takes Borrow<UdpSocket> #3451

Conversation

leshow commented Jan 20, 2021 • edited Loading

Motivation

Possible Solution?

Choose a reason for hiding this comment

leshow Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leshow Mar 27, 2021 • edited Loading

Choose a reason for hiding this comment

Darksonn commented Mar 27, 2021

leshow commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leshow Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leshow Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leshow commented Jan 20, 2021 •

edited

Loading

leshow Jan 28, 2021 •

edited

Loading

leshow Mar 27, 2021 •

edited

Loading

leshow Apr 12, 2021 •

edited

Loading

leshow Apr 12, 2021 •

edited

Loading