fix(quic): use Provider::send_to for UDP datagram #28
Conversation
| fn from_std_udp_socket(socket: std::net::UdpSocket) -> io::Result<Self::UdpSocket> { | ||
| Ok(socket.into()) | ||
| } | ||
|
|
||
| fn send_to<'a>( | ||
| udp_socket: &'a Self::UdpSocket, | ||
| buf: &'a [u8], | ||
| target: std::net::SocketAddr, | ||
| ) -> BoxFuture<'a, io::Result<usize>> { | ||
| udp_socket.send_to(buf, target).boxed() | ||
| } |
There was a problem hiding this comment.
Instead of two functions, we could also make one function that accepts an std::net::UdpSocket, converts it internally to the runtime-specific one, sends the packet and returns the socket.
That would avoid the associated type and reduce this to one function that needs to be implemented.
|
Appreciate the reviews @kpp and @thomaseizinger. Latest commit applies the above suggestions and hides the Tokio and async-std types. Mind taking another look? |
| target: std::net::SocketAddr, | ||
| ) -> BoxFuture<'a, io::Result<usize>> { | ||
| Box::pin(async move { | ||
| async_std::net::UdpSocket::from(udp_socket.try_clone()?) |
There was a problem hiding this comment.
When does try_clone fail? The docs don't mention anything.
There was a problem hiding this comment.
My understanding:
- A
UdpSocketboils down to a file descriptor. See e.g. https://doc.rust-lang.org/src/std/sys/unix/net.rs.html#30 - "Duplicating" (i.e. cloning) a file descriptor can fail due to various reasons, e.g. the underlying fail being closed or the process is running out of file descriptors.
I am assuming that you wonder whether we can do without propagating the error. Given that we already need a way to propagate the error from send_to, I don't think we should apply any special treatment to the error from try_clone. Are you fine with that @thomaseizinger?
There was a problem hiding this comment.
Sounds good to me! I was just wondering whether there are any specific error cases :)
Happy to keep as is!
| if let Err(e) = P::send_to(&socket, &contents, remote_addr).await { | ||
| return Error::Io(e); | ||
| } |
There was a problem hiding this comment.
Isn't this just what ? would do?
There was a problem hiding this comment.
I wondered the same. Note the function signature:
async fn punch_holes<P: Provider>(socket: UdpSocket, remote_addr: SocketAddr) -> Error {The function never returns, unless there is an Error. In words, the function returns Error and not Result<_, Error>. Thus one can not use ?.
There was a problem hiding this comment.
Perhaps we should change it to Result<Infallible, Error>?
There was a problem hiding this comment.
We discussed this in libp2p#3964 as well, can't find the concrete conversation right now. I suggest we keep as is unless you feel strongly about it @thomaseizinger.
Description
Links to any relevant issues
Addresses libp2p#3454 (comment).
Open Questions
Change checklist