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

util: Refactor UdpFramed & add UdpFramedRead/UdpFramedWrite #3086

Closed
wants to merge 5 commits into from

Conversation

leshow
Copy link
Contributor

@leshow leshow commented Nov 2, 2020

Motivation

Framed has both a FramedRead and FramedWrite, it's split up nicely to use the same underlying implementation. This doesn't exist for udp

Solution

I basically copied the same style but for UdpFramed, so that you can make both a Stream-only or Sink-only. It's not totally cleaned up yet, I wanted to get feedback on if this would be accepted first if I keep working on it.

I would love to make the underlying type generic and not UdpSocket or have something generic that bounds over types that implement poll_send & poll_recv, but unless you're open to adding such a trait, the type will have to be monomorphic, I think.

edit: this builds on #3044 so that needs to be merged first

@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate C-enhancement Category: A PR with an enhancement or bugfix. M-codec Module: tokio-util/codec labels Nov 2, 2020
@leshow
Copy link
Contributor Author

leshow commented Nov 12, 2020

I've added some docs & tests to this. It's the same implementation as before just refactored to be more in line with the other types in tokio-util. It also removes the Unpin bound.

@leshow leshow force-pushed the udp_framed_read_write branch 2 times, most recently from fbd6442 to bcb5b3e Compare November 12, 2020 17:32
@leshow leshow changed the title util: Add UdpFramedRead/UdpFramedWrite util: Refactor UdpFramed & add UdpFramedRead/UdpFramedWrite Nov 24, 2020
@Darksonn
Copy link
Contributor

Darksonn commented Dec 5, 2020

Sorry, I haven't gotten around to this because it's so long..

const INITIAL_WR_CAPACITY: usize = 8 * 1024;

impl<C: Decoder + Unpin> Stream for UdpFramed<C> {
impl<C> Stream for UdpFramed<C>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes Unpin bound-- is this okay?

pub(crate) const INITIAL_RD_CAPACITY: usize = 64 * 1024;
pub(crate) const INITIAL_WR_CAPACITY: usize = 8 * 1024;

impl<C, R> Stream for UdpFramedImpl<C, R>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original Stream impl for UdpFramed moved here

@leshow
Copy link
Contributor Author

leshow commented Dec 5, 2020

Oh, that's fine. It looks like a lot of changes but it mostly just shuffles things around to be similar to Framed/FramedRead/FramedWrite, with the major difference being it's not parameterized over T: AsyncRead + AsyncWrite and instead holds a UdpSocket.

I added a few comments to the PR that I wanted to call your attention too, but take your time!

@leshow leshow force-pushed the udp_framed_read_write branch 4 times, most recently from 8e96d88 to f73c68b Compare January 3, 2021 16:28
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-enhancement Category: A PR with an enhancement or bugfix. M-codec Module: tokio-util/codec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants