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.

Sign up for GitHub

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

fix(quic): use Provider::send_to for UDP datagram #28

Merged
merged 2 commits into from
Jul 4, 2023
Merged

fix(quic): use Provider::send_to for UDP datagram #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
459dedd
fix(quic): use Provider::send_to for UDP datagram
mxinden Jun 28, 2023
f31da2e
Simplify and hide dependency types
mxinden Jul 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions transports/quic/src/hole_punching.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use futures::future::Either;
use rand::{distributions, Rng};

use std::{
io,
net::{SocketAddr, UdpSocket},
time::Duration,
};
Expand Down Expand Up @@ -33,10 +32,8 @@ async fn punch_holes<P: Provider>(socket: UdpSocket, remote_addr: SocketAddr) ->
.take(64)
.collect();

if let Err(e) = socket.send_to(&contents, remote_addr) {
if !matches!(e.kind(), io::ErrorKind::WouldBlock) {
return Error::Io(e);
}
if let Err(e) = P::send_to(&socket, &contents, remote_addr).await {
return Error::Io(e);
}
Comment on lines +35 to 37
Copy link

Choose a reason for hiding this comment

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

Isn't this just what ? would do?

Copy link
Author

Choose a reason for hiding this comment

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

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 ?.

Copy link

Choose a reason for hiding this comment

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

Perhaps we should change it to Result<Infallible, Error>?

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
}
8 changes: 8 additions & 0 deletions transports/quic/src/provider.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use futures::{future::BoxFuture, Future};
use if_watch::IfEvent;
use std::{
io,
net::{SocketAddr, UdpSocket},
task::{Context, Poll},
time::Duration,
};
Expand Down Expand Up @@ -62,4 +63,11 @@ pub trait Provider: Unpin + Send + Sized + 'static {

/// Sleep for specified amount of time.
fn sleep(duration: Duration) -> BoxFuture<'static, ()>;

/// Sends data on the socket to the given address. On success, returns the number of bytes written.
fn send_to<'a>(
udp_socket: &'a UdpSocket,
buf: &'a [u8],
target: SocketAddr,
) -> BoxFuture<'a, io::Result<usize>>;
}
13 changes: 13 additions & 0 deletions transports/quic/src/provider/async_std.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use async_std::task::spawn;
use futures::{future::BoxFuture, Future, FutureExt};
use std::{
io,
net::UdpSocket,
task::{Context, Poll},
time::Duration,
};
Expand Down Expand Up @@ -59,4 +60,16 @@ impl super::Provider for Provider {
fn sleep(duration: Duration) -> BoxFuture<'static, ()> {
async_std::task::sleep(duration).boxed()
}

fn send_to<'a>(
udp_socket: &'a UdpSocket,
buf: &'a [u8],
target: std::net::SocketAddr,
) -> BoxFuture<'a, io::Result<usize>> {
Box::pin(async move {
async_std::net::UdpSocket::from(udp_socket.try_clone()?)
Copy link

Choose a reason for hiding this comment

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

When does try_clone fail? The docs don't mention anything.

Copy link
Author

Choose a reason for hiding this comment

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

My understanding:

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?

Copy link

Choose a reason for hiding this comment

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

Sounds good to me! I was just wondering whether there are any specific error cases :)

Happy to keep as is!

.send_to(buf, target)
.await
})
}
}
13 changes: 13 additions & 0 deletions transports/quic/src/provider/tokio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use futures::{future::BoxFuture, Future, FutureExt};
use std::{
io,
net::{SocketAddr, UdpSocket},
task::{Context, Poll},
time::Duration,
};
Expand Down Expand Up @@ -58,4 +59,16 @@ impl super::Provider for Provider {
fn sleep(duration: Duration) -> BoxFuture<'static, ()> {
tokio::time::sleep(duration).boxed()
}

fn send_to<'a>(
udp_socket: &'a UdpSocket,
buf: &'a [u8],
target: SocketAddr,
) -> BoxFuture<'a, io::Result<usize>> {
Box::pin(async move {
tokio::net::UdpSocket::from_std(udp_socket.try_clone()?)?
.send_to(buf, target)
.await
})
}
}