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

Figure out plans for UdpFramed #2830

Closed
seanmonstar opened this issue Sep 10, 2020 · 10 comments
Closed

Figure out plans for UdpFramed #2830

seanmonstar opened this issue Sep 10, 2020 · 10 comments
Labels
A-tokio-util Area: The tokio-util crate C-musing Category: musings about a better world M-codec Module: tokio-util/codec

Comments

@seanmonstar
Copy link
Member

The UdpFramed type in tokio-util was disabled #2828, because the implementation depended on poll_send_to and poll_recv_from methods, which were removed. We need to figure out what do with this type going forward. Some options:

  • Completely remove UdpFramed.
  • Add back undocumented poll_send_to and poll_recv_from methods that behave similar to AsyncRead and AsyncWrite internally.
@seanmonstar seanmonstar added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Sep 10, 2020
@seanmonstar seanmonstar added this to the v0.3 milestone Sep 10, 2020
@Darksonn Darksonn added the C-musing Category: musings about a better world label Sep 11, 2020
@leshow
Copy link
Contributor

leshow commented Sep 16, 2020

Chiming in as a user of UdpFramed. I wasn't aware that the poll_ fns were removed from UdpSocket, is there still a way to call recv from a Poll returning fn? I guess one could still call recv_from(..).poll_unpin(cx) ?

One unfortunate thing about UdpFramed is that it ceases to work if you split() a UdpSocket since it requires a concrete type that won't accept a RecvHalf. There's no ReadFramed type of dichotomy in the udp types. I realize the docs for UdpFramed points to calling split after the fact, but AFAIK this is the split() from the futures_util library, not tokio, which uses their BiLock, whereas the UdpSocket::split just creates an Arc and seemed more favourable. In some cases this had lead me to create my own UdpReadFramed based off the implementation of UdpFramed.

edit: all of the above doesn't make much sense anymore since the removal of split

In any case I think UdpFramed has some value, would be nice if it were generic though.

Am I correct in thinking that if it were removed there would be no real 'recommended path' for getting from a UdpSocket to a Stream? Especially with the removal of the aforementioned poll_ methods?

@carllerche
Copy link
Member

IIRC, the conclusion is to expose poll_* functions in tokio and to commit to supporting them in 1.0.

@carllerche
Copy link
Member

The remaining 0.3 steps are tracked in #2928. I am removing the 0.3 milestone from this issue but leaving it open. We still need to bring back UdpFramed.

@carllerche carllerche removed this from the v0.3 milestone Oct 8, 2020
@leshow
Copy link
Contributor

leshow commented Oct 16, 2020

Would you like to bring it back more or less like it was before, just updated to work with the new apis? Which is to say, concurrent send/recv should be pretty simple. I can submit a PR for something like this if you want.

@Darksonn
Copy link
Contributor

@leshow You would have to add poll_* functions to UdpSocket to make it possible to implement, but yes.

rkusa added a commit to rkusa/DATIS that referenced this issue Oct 27, 2020
tokio cannot be updated yet, since UdpFramed got removed, see
tokio-rs/tokio#2830
@leshow
Copy link
Contributor

leshow commented Nov 2, 2020

@rkusa I got a comment in my email asking about what you can do without UdpFramed if you want to make a Stream, I don't see the message here though. I just wanted to add that now that poll_ methods are back in tokio 3.2, UdpFramed isn't the only way to get a Stream. You can simply implement Stream yourself for a type, that's an option.

If you don't want a Stream impl specifically, but just want to turn a UdpSocket into a bunch of frames, there's nothing stopping you from wrapping UdpSocket in another type and adding a async fn method to decode a frame, then calling that in some kind of a loop.

Of course, the PR to add UdpFramed is just waiting to be merged, so you can totally just wait for the next release too.

@MagicRB
Copy link

MagicRB commented Nov 2, 2020

Yeah, I wrote that commend, then I realized there already is a PR so I deleted it, sorry for the trouble

@tbraun96
Copy link

tbraun96 commented Dec 3, 2020

UdpFramed should definitely be added. Crates I'm upgrading depend on it. For now, having to use a thin wrapper

@leshow
Copy link
Contributor

leshow commented Dec 4, 2020

UdpFramed is added and has been merged, this issue can probably be closed?

there is #3086 that refactors and adds read & write only versions of UdpFramed also though

@Darksonn
Copy link
Contributor

Darksonn commented Dec 5, 2020

Yeah, it is available as tokio_util::udp::UdpFramed in the latest version of tokio-util. Closing.

@Darksonn Darksonn closed this as completed Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-musing Category: musings about a better world M-codec Module: tokio-util/codec
Projects
None yet
Development

No branches or pull requests

6 participants