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: introduce split on UnixDatagram #2557

Merged
merged 5 commits into from
Jul 24, 2020
Merged

Conversation

cssivision
Copy link
Contributor

@cssivision cssivision commented May 22, 2020

fix issue #2551

this pr can be closed if this is merged. #1757

@Darksonn 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 May 22, 2020
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.

I only had a quick look, but three things came to mind:

  1. What about a by-ref split like in TcpStream?
  2. What about naming? TcpStream calls its owned split into_split.
  3. The owned tcp split closes the stream when the writer is dropped. Should we also do that here?

We currently have different naming conventions on split/into_split that we can't fix until v0.3 due to backwards compatibility. Still doesn't hurt to choose what we intend to converge on in v0.3 here.

See also the PR for owned split on TCP streams: #2270.

@cssivision
Copy link
Contributor Author

cssivision commented May 23, 2020

I only had a quick look, but three things came to mind:

  1. What about a by-ref split like in TcpStream?
  2. What about naming? TcpStream calls its owned split into_split.
  3. The owned tcp split closes the stream when the writer is dropped. Should we also do that here?

We currently have different naming conventions on split/into_split that we can't fix until v0.3 due to backwards compatibility. Still doesn't hurt to choose what we intend to converge on in v0.3 here.

See also the PR for owned split on TCP streams: #2270.

thanks for reviewing this pr. keep consistent with TcpStream's into_split is a good idea.

Is it necessary to add into_split for UdpSocket and UnixStream, and add split for UnixDatagram? if it's yes, i will fire another pr for this.

@Darksonn
Copy link
Contributor

The issue with UdpSocket and UnixStream is that the function they already have called split does what TcpStream's into_split does, and that can only be fixed when we move to v0.3.

@Darksonn Darksonn requested a review from Matthias247 May 23, 2020 15:32
@cssivision
Copy link
Contributor Author

cssivision commented May 23, 2020

it's ok to fix UnixStream and UdpSocket when reach v0.3. I will add split to UnixDatagram in another pr.

@cssivision
Copy link
Contributor Author

activate

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.

This looks reasonable at a first glance.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@carllerche carllerche merged commit ff7125e into tokio-rs:master Jul 24, 2020
@Darksonn Darksonn mentioned this pull request Nov 10, 2020
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 C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants