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

feat(quinn,quinn-udp): Introduce net feature to allow disabling socket2 and std::net::UdpSocket dependencies #2037

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
42 changes: 16 additions & 26 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,45 +73,35 @@ jobs:
name: test wasm32-unknown-unknown
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v4

- name: Install stable toolchain
uses: dtolnay/rust-toolchain@stable

- name: Add wasm target
run: rustup target add wasm32-unknown-unknown

- name: Install nodejs v20
uses: actions/setup-node@v4
- uses: actions/checkout@v4
djc marked this conversation as resolved.
Show resolved Hide resolved
- uses: dtolnay/rust-toolchain@stable
- run: rustup target add wasm32-unknown-unknown
- uses: actions/setup-node@v4
with:
node-version: 20
- uses: bytecodealliance/actions/wasm-tools/setup@v1
- uses: cargo-bins/cargo-binstall@main

- name: Setup `wasm-tools`
uses: bytecodealliance/actions/wasm-tools/setup@v1

- name: Install cargo binstall
uses: cargo-bins/cargo-binstall@main

# We need to downgrade cc to version 1.1.31 for ring Wasm compilation to work.
# See the upstream issue https://github.com/rust-lang/cc-rs/issues/1275
- name: Use working `cc` version 1.1.31
- name: Downgrade `cc` to version 1.1.31
run: cargo update -p cc --precise 1.1.31

- name: build wasm32 tests (quinn-proto)
run: cargo test -p quinn-proto --target wasm32-unknown-unknown --no-run
- run: cargo test -p quinn-proto --target wasm32-unknown-unknown --no-run
- run: cargo check -p quinn-udp --target wasm32-unknown-unknown --no-default-features --features=tracing,log
- run: cargo rustc -p quinn --target wasm32-unknown-unknown --no-default-features --features=rustls --crate-type=cdylib

# If the Wasm file contains any 'import "env"' declarations, then
# some non-Wasm-compatible code made it into the final code.
- name: Check for 'import "env"' in Wasm
- name: Ensure no 'import "env"' in quinn_proto Wasm
run: |
! wasm-tools print --skeleton target/wasm32-unknown-unknown/debug/deps/quinn_proto-*.wasm | grep 'import "env"'
- name: Ensure no 'import "env"' in quinn Wasm
run: |
! wasm-tools print --skeleton target/wasm32-unknown-unknown/debug/quinn.wasm | grep 'import "env"'
- name: Install wasm-bindgen-test-runner
run: cargo binstall wasm-bindgen-cli --locked --no-confirm

- name: wasm32 test (quinn-proto)
run: cargo test -p quinn-proto --target wasm32-unknown-unknown
- run: cargo binstall wasm-bindgen-cli --locked --no-confirm
- run: cargo test -p quinn-proto --target wasm32-unknown-unknown

msrv:
runs-on: ubuntu-latest
Expand Down
8 changes: 6 additions & 2 deletions quinn-udp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,27 @@ workspace = ".."
all-features = true

[features]
default = ["tracing", "log"]
default = ["tracing", "log", "net"]
# Configure `tracing` to log events via `log` if no `tracing` subscriber exists.
log = ["tracing/log"]
direct-log = ["dep:log"]
# Use private Apple APIs to send multiple packets in a single syscall.
fast-apple-datapath = []
net = ["dep:socket2"]
matheus23 marked this conversation as resolved.
Show resolved Hide resolved

[dependencies]
libc = "0.2.158"
log = { workspace = true, optional = true }
socket2 = { workspace = true }
socket2 = { workspace = true, optional = true }
tracing = { workspace = true, optional = true }

[target.'cfg(windows)'.dependencies]
once_cell = { workspace = true }
windows-sys = { workspace = true }

[target.'cfg(all(target_family = "wasm", target_os = "unknown"))'.dependencies]
web-time = { workspace = true }

[dev-dependencies]
criterion = { version = "0.5", default-features = false, features = ["async_tokio"] }
tokio = { workspace = true, features = ["rt", "rt-multi-thread", "net"] }
Expand Down
45 changes: 29 additions & 16 deletions quinn-udp/src/lib.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of the many cfg(feature = "net"), wouldn't something along the following be simpler?

#[cfg(feature = "net")]
use net::*;

#[cfg(feature = "net")]
mod net {
  // Everything that was previously feature flagged behind net.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a version where I've experimented with that.
It's a much more invasive change, I'm not sure if it's much better?
I can add it to this PR if other people agree: n0-computer@d06b71b

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the commit. I didn't think of the fact that unix.rs and windows.rs would need to be under the net/ module.

At this point I don't have a better idea, nor a preference for the options listed here.

Choose a reason for hiding this comment

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

couldnt windows and unix stuff be stuffed under a "system" "sys" or something like that.

Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,35 @@
#![warn(unreachable_pub)]
#![warn(clippy::use_self)]

#[cfg(unix)]
use std::net::{IpAddr, Ipv6Addr, SocketAddr};
djc marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(all(feature = "net", unix))]
use std::os::unix::io::AsFd;
#[cfg(windows)]
#[cfg(all(feature = "net", windows))]
use std::os::windows::io::AsSocket;
use std::{
net::{IpAddr, Ipv6Addr, SocketAddr},
sync::Mutex,
time::{Duration, Instant},
};

#[cfg(any(unix, windows))]
#[cfg(feature = "net")]
use std::sync::Mutex;
// Allowing unused, otherwise the cfg condition complexity gets
// out of control only to skip an unused `Duration` import.
#[allow(unused)]
#[cfg(not(all(target_family = "wasm", target_os = "unknown")))]
use std::time::{Duration, Instant};
#[allow(unused)]
#[cfg(all(target_family = "wasm", target_os = "unknown"))]
use web_time::{Duration, Instant};

#[cfg(all(feature = "net", any(unix, windows)))]
mod cmsg;

#[cfg(unix)]
#[cfg(all(feature = "net", unix))]
#[path = "unix.rs"]
mod imp;

#[cfg(windows)]
#[cfg(all(feature = "net", windows))]
#[path = "windows.rs"]
mod imp;

// No ECN support
#[cfg(not(any(unix, windows)))]
#[cfg(all(feature = "net", not(any(unix, windows))))]
#[path = "fallback.rs"]
mod imp;

Expand All @@ -76,10 +82,15 @@ mod log {
pub(crate) use no_op::*;
}

#[cfg(feature = "net")]
pub use imp::UdpSocketState;

/// Number of UDP packets to send/receive at a time
#[cfg(feature = "net")]
pub const BATCH_SIZE: usize = imp::BATCH_SIZE;
/// Number of UDP packets to send/receive at a time
#[cfg(not(feature = "net"))]
pub const BATCH_SIZE: usize = 1;

/// Metadata for a single buffer filled with bytes received from the network
///
Expand Down Expand Up @@ -141,13 +152,14 @@ pub struct Transmit<'a> {
}

/// Log at most 1 IO error per minute
#[cfg(feature = "net")]
const IO_ERROR_LOG_INTERVAL: Duration = std::time::Duration::from_secs(60);

/// Logs a warning message when sendmsg fails
///
/// Logging will only be performed if at least [`IO_ERROR_LOG_INTERVAL`]
/// has elapsed since the last error was logged.
#[cfg(any(feature = "tracing", feature = "direct-log"))]
#[cfg(all(feature = "net", any(feature = "tracing", feature = "direct-log")))]
fn log_sendmsg_error(
last_send_error: &Mutex<Instant>,
err: impl core::fmt::Debug,
Expand All @@ -164,17 +176,18 @@ fn log_sendmsg_error(
}

// No-op
#[cfg(not(any(feature = "tracing", feature = "direct-log")))]
#[cfg(all(feature = "net", not(any(feature = "tracing", feature = "direct-log"))))]
fn log_sendmsg_error(_: &Mutex<Instant>, _: impl core::fmt::Debug, _: &Transmit) {}

/// A borrowed UDP socket
///
/// On Unix, constructible via `From<T: AsFd>`. On Windows, constructible via `From<T:
/// AsSocket>`.
// Wrapper around socket2 to avoid making it a public dependency and incurring stability risk
#[cfg(feature = "net")]
pub struct UdpSockRef<'a>(socket2::SockRef<'a>);

#[cfg(unix)]
#[cfg(all(feature = "net", unix))]
impl<'s, S> From<&'s S> for UdpSockRef<'s>
where
S: AsFd,
Expand All @@ -184,7 +197,7 @@ where
}
}

#[cfg(windows)]
#[cfg(all(feature = "net", windows))]
impl<'s, S> From<&'s S> for UdpSockRef<'s>
where
S: AsSocket,
Expand Down
14 changes: 9 additions & 5 deletions quinn/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ rust-version.workspace = true
all-features = true

[features]
default = ["log", "platform-verifier", "runtime-tokio", "rustls-ring"]
default = ["log", "platform-verifier", "net", "runtime-tokio", "rustls-ring"]
# Enables `Endpoint::client` and `Endpoint::server` conveniences
aws-lc-rs = ["proto/aws-lc-rs"]
aws-lc-rs-fips = ["proto/aws-lc-rs-fips"]
Expand All @@ -32,9 +32,10 @@ rustls-aws-lc-rs-fips = ["dep:rustls", "aws-lc-rs-fips", "proto/rustls-aws-lc-rs
rustls-ring = ["dep:rustls", "ring", "proto/rustls-ring", "proto/ring"]
# Enables `Endpoint::client` and `Endpoint::server` conveniences
ring = ["proto/ring"]
runtime-tokio = ["tokio/time", "tokio/rt", "tokio/net"]
runtime-async-std = ["async-io", "async-std"]
runtime-smol = ["async-io", "smol"]
runtime-tokio = ["net", "tokio/time", "tokio/rt", "tokio/net"]
runtime-async-std = ["net", "async-io", "async-std"]
runtime-smol = ["net", "async-io", "smol"]
net = ["dep:socket2", "udp/net"]

# Configure `tracing` to log events via `log` if no `tracing` subscriber exists.
log = ["tracing/log", "proto/log", "udp/log"]
Expand All @@ -52,12 +53,15 @@ pin-project-lite = { workspace = true }
proto = { package = "quinn-proto", path = "../quinn-proto", version = "0.11.7", default-features = false }
rustls = { workspace = true, optional = true }
smol = { workspace = true, optional = true }
socket2 = { workspace = true }
socket2 = { workspace = true, optional = true }
thiserror = { workspace = true }
tracing = { workspace = true }
tokio = { workspace = true }
udp = { package = "quinn-udp", path = "../quinn-udp", version = "0.5", default-features = false, features = ["tracing"] }

[target.'cfg(all(target_family = "wasm", target_os = "unknown"))'.dependencies]
web-time = { workspace = true }

[dev-dependencies]
anyhow = { workspace = true }
crc = { workspace = true }
Expand Down
3 changes: 1 addition & 2 deletions quinn/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::{
pin::Pin,
sync::Arc,
task::{Context, Poll, Waker},
time::{Duration, Instant},
};

use bytes::Bytes;
Expand All @@ -22,7 +21,7 @@ use crate::{
recv_stream::RecvStream,
runtime::{AsyncTimer, AsyncUdpSocket, Runtime, UdpPoller},
send_stream::SendStream,
udp_transmit, ConnectionEvent, VarInt,
udp_transmit, ConnectionEvent, Duration, Instant, VarInt,
};
use proto::{
congestion::Controller, ConnectionError, ConnectionHandle, ConnectionStats, Dir, EndpointEvent,
Expand Down
13 changes: 7 additions & 6 deletions quinn/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@ use std::{
str,
sync::{Arc, Mutex},
task::{Context, Poll, Waker},
time::Instant,
};

#[cfg(any(feature = "aws-lc-rs", feature = "ring"))]
#[cfg(all(feature = "net", any(feature = "aws-lc-rs", feature = "ring")))]
use crate::runtime::default_runtime;
use crate::{
runtime::{AsyncUdpSocket, Runtime},
udp_transmit,
udp_transmit, Instant,
};
use bytes::{Bytes, BytesMut};
use pin_project_lite::pin_project;
Expand All @@ -25,7 +24,7 @@ use proto::{
EndpointEvent, ServerConfig,
};
use rustc_hash::FxHashMap;
#[cfg(any(feature = "aws-lc-rs", feature = "ring"))]
#[cfg(all(feature = "net", any(feature = "aws-lc-rs", feature = "ring"),))]
use socket2::{Domain, Protocol, Socket, Type};
use tokio::sync::{futures::Notified, mpsc, Notify};
use tracing::{Instrument, Span};
Expand Down Expand Up @@ -67,7 +66,7 @@ impl Endpoint {
///
/// Some environments may not allow creation of dual-stack sockets, in which case an IPv6
/// client will only be able to connect to IPv6 servers. An IPv4 client is never dual-stack.
#[cfg(any(feature = "aws-lc-rs", feature = "ring"))] // `EndpointConfig::default()` is only available with these
#[cfg(all(feature = "net", any(feature = "aws-lc-rs", feature = "ring")))] // `EndpointConfig::default()` is only available with these
pub fn client(addr: SocketAddr) -> io::Result<Self> {
let socket = Socket::new(Domain::for_address(addr), Type::DGRAM, Some(Protocol::UDP))?;
if addr.is_ipv6() {
Expand Down Expand Up @@ -97,7 +96,7 @@ impl Endpoint {
/// IPv6 address on Windows will not by default be able to communicate with IPv4
/// addresses. Portable applications should bind an address that matches the family they wish to
/// communicate within.
#[cfg(any(feature = "aws-lc-rs", feature = "ring"))] // `EndpointConfig::default()` is only available with these
#[cfg(all(feature = "net", any(feature = "aws-lc-rs", feature = "ring")))] // `EndpointConfig::default()` is only available with these
pub fn server(config: ServerConfig, addr: SocketAddr) -> io::Result<Self> {
let socket = std::net::UdpSocket::bind(addr)?;
let runtime = default_runtime()
Expand All @@ -111,6 +110,7 @@ impl Endpoint {
}

/// Construct an endpoint with arbitrary configuration and socket
#[cfg(feature = "net")]
pub fn new(
config: EndpointConfig,
server_config: Option<ServerConfig>,
Expand Down Expand Up @@ -234,6 +234,7 @@ impl Endpoint {
/// Switch to a new UDP socket
///
/// See [`Endpoint::rebind_abstract()`] for details.
#[cfg(feature = "net")]
pub fn rebind(&self, socket: std::net::UdpSocket) -> io::Result<()> {
self.rebind_abstract(self.runtime.wrap_udp_socket(socket)?)
}
Expand Down
7 changes: 6 additions & 1 deletion quinn/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
#![warn(unreachable_pub)]
#![warn(clippy::use_self)]

use std::{sync::Arc, time::Duration};
use std::sync::Arc;

macro_rules! ready {
($e:expr $(,)?) => {
Expand All @@ -61,6 +61,11 @@ mod runtime;
mod send_stream;
mod work_limiter;

#[cfg(not(all(target_family = "wasm", target_os = "unknown")))]
pub(crate) use std::time::{Duration, Instant};
#[cfg(all(target_family = "wasm", target_os = "unknown"))]
pub(crate) use web_time::{Duration, Instant};

pub use proto::{
congestion, crypto, AckFrequencyConfig, ApplicationClose, Chunk, ClientConfig, ClosedStream,
ConfigError, ConnectError, ConnectionClose, ConnectionError, ConnectionStats, EndpointConfig,
Expand Down
6 changes: 2 additions & 4 deletions quinn/src/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@ use std::{
#[cfg(feature = "lock_tracking")]
mod tracking {
use super::*;
use std::{
collections::VecDeque,
time::{Duration, Instant},
};
use crate::{Duration, Instant};
use std::collections::VecDeque;
use tracing::warn;

#[derive(Debug)]
Expand Down
4 changes: 3 additions & 1 deletion quinn/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,20 @@ use std::{
pin::Pin,
sync::Arc,
task::{Context, Poll},
time::Instant,
};

use udp::{RecvMeta, Transmit};

use crate::Instant;

/// Abstracts I/O and timer operations for runtime independence
pub trait Runtime: Send + Sync + Debug + 'static {
/// Construct a timer that will expire at `i`
fn new_timer(&self, i: Instant) -> Pin<Box<dyn AsyncTimer>>;
/// Drive `future` to completion in the background
fn spawn(&self, future: Pin<Box<dyn Future<Output = ()> + Send>>);
/// Convert `t` into the socket type used by this runtime
#[cfg(feature = "net")]
matheus23 marked this conversation as resolved.
Show resolved Hide resolved
fn wrap_udp_socket(&self, t: std::net::UdpSocket) -> io::Result<Arc<dyn AsyncUdpSocket>>;
/// Look up the current time
///
Expand Down
Loading