Skip to content

Use socket2::Socket for UdpSocket #1431

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

Closed

Conversation

Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw commented Jan 5, 2021

This includes a single breaking change which involves multiple
UdpSocket methods: the receive methods now accept an uninitialised
buffer ([MaybeUninit<u8>]), rather than an initialised one. This is the
case of the following methods:

  • UdpSocket::recv_from
  • UdpSocket::peek_from
  • UdpSocket::recv
  • UdpSocket::peek

This allows higher level libraries, such as Tokio or Heph, to use
uninitialised buffers if they want to. Note that the API guarantees not
to write any uninitialised data to the buffer, so it's not unsound to
pass &[u8] to the methods.

Finally this adds a debug assertions to UdpSocket::recv_from that checks
if the buffer passed isn't zero sized. Passing a zero sized buffer to
recv_from causes the OS to not initialise the source address (at least
on macOS), which in turn causes a misleading error to be returned saying
the address family is invalid. Before this change didn't check this and
likely returned an invalid IPv6 address.

Updates #1381

@Thomasdezeeuw
Copy link
Collaborator Author

Depends on #1432 to update the Rustc version.

@Thomasdezeeuw Thomasdezeeuw force-pushed the issue_1381_v0.8_use-socket2 branch from 0cc8ffd to ef5af1b Compare January 19, 2021 17:54
This includes a single breaking change which involves multiple UdpSocket
methods: the receive methods now accept an uninitialised buffer
([MaybeUninit<u8>]), rather than an initialised one. This is the case of
the following methods:

* UdpSocket::recv_from
* UdpSocket::peek_from
* UdpSocket::recv
* UdpSocket::peek

This allows higher level libraries, such as Tokio or Heph, to use
uninitialised buffers if they want to. Note that the API guarantees not
to write any uninitialised data to the buffer, so it's not unsound to
pass `&[u8]` to the methods.

Finally this adds a debug assertions to UdpSocket::recv_from that checks
if the buffer passed isn't zero sized. Passing a zero sized buffer to
recv_from causes the OS to not initialise the source address (at least
on macOS), which in turn causes a misleading error to be returned saying
the address family is invalid. Before this change didn't check this and
likely returned an invalid IPv6 address.
@Thomasdezeeuw Thomasdezeeuw force-pushed the issue_1381_v0.8_use-socket2 branch from ef5af1b to 8f6e958 Compare January 19, 2021 18:01
@carllerche
Copy link
Member

Thanks for doing this. There should be a high bar to add a new dependency to the Tokio stack and I am not sure socket2 meets that bar yet.

Tokio is aiming to limit the number of transitive dependencies it pulls in. A common complaint we hear from users is that depending on Tokio pulls in too many crates. Each new dependency needs to be weighed for value in the stack as it would take budget away from other areas of Tokio. My understanding of this goal is to reduce the maintenance overhead between mio & socket2, which I understand and you are doing a great job with your work on Mio & socket2. However, what is the end value to the user? Does adding a dependency on socket2 bring enough value compared to another hypothetical dependency?

Second, crates we depend on need to have clear ownership. Who owns the crate (individual, organization, ...)? Who can publish versions of the crate? How are security reports handled? Who reviews / approves PRs? What happens when the maintainer steps down? Who takes their place? Adding new transitive dependency with a separate ownership structure increases a user's blast radius when depending on Tokio.

I see socket2 lives in the rust-lang organization, but it does not appear to be maintained by the rust-lang libs team? Does the rust-lang libs team have a policy w.r.t. to maintenance & process around crates in the org?

Before adding a dependency, I think we should work through these thoughts and probably define a policy for deciding whether or not we add new dependencies.

@Thomasdezeeuw
Copy link
Collaborator Author

Thanks for doing this. There should be a high bar to add a new dependency to the Tokio stack and I am not sure socket2 meets that bar yet.

Tokio is aiming to limit the number of transitive dependencies it pulls in. A common complaint we hear from users is that depending on Tokio pulls in too many crates. Each new dependency needs to be weighed for value in the stack as it would take budget away from other areas of Tokio. My understanding of this goal is to reduce the maintenance overhead between mio & socket2, which I understand and you are doing a great job with your work on Mio & socket2. However, what is the end value to the user? Does adding a dependency on socket2 bring enough value compared to another hypothetical dependency?

The end value to the end user is additional socket options added to the net types TcpStream, UdpSocket, etc. I've accepted TcpSocket as stop-gap solution in v0.7, but I'm going to remove it for v0.8. Either users (Tokio included) can implement this themselves, use a crate like socket2 or Mio can provide it by using socket2.

Second, crates we depend on need to have clear ownership. Who owns the crate (individual, organization, ...)? Who can publish versions of the crate? How are security reports handled? Who reviews / approves PRs? What happens when the maintainer steps down? Who takes their place? Adding new transitive dependency with a separate ownership structure increases a user's blast radius when depending on Tokio.

These are all very valid questions and have few (good enough) answers, but the same holds true for Mio itself. The short answer is for both Mio and socket2 is that I handle it all. But I think its a good idea to add some structure for this, both to Mio and Socket2.

I see socket2 lives in the rust-lang organization, but it does not appear to be maintained by the rust-lang libs team? Does the rust-lang libs team have a policy w.r.t. to maintenance & process around crates in the org?

Before adding a dependency, I think we should work through these thoughts and probably define a policy for deciding whether or not we add new dependencies.

All valid question to which I don't have good answers. Still I don't think it should block this specific dependency as I maintain both crates. Furthermore although a policy for adding dependency would be great we don't have one now, and if we have such a policy would any of our dependencies pass the bar?

Take miow for example (although I'm not trying to pick on miow or any of its contributors), faern (not pinging on purpose) got write permission to crates.io to publish a fix for the SockAddr layout issue: yoshuawuyts/miow#39 (comment). Would such an action be acceptable by any of our hypothetical policies..?

@Thomasdezeeuw
Copy link
Collaborator Author

We're not adding socket2 as Mio dependency.

@Thomasdezeeuw Thomasdezeeuw deleted the issue_1381_v0.8_use-socket2 branch September 28, 2021 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants