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 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
40 changes: 15 additions & 25 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -144,40 +144,30 @@ 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

- 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=log,platform-verifier,rustls-ring --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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ url = "2"
wasm-bindgen-test = { version = "0.3.45" }
web-time = "1"
windows-sys = { version = ">=0.52, <=0.59", features = ["Win32_Foundation", "Win32_System_IO", "Win32_Networking_WinSock"] }
cfg_aliases = "0.2"

[profile.bench]
debug = true
Expand Down
6 changes: 4 additions & 2 deletions quinn-udp/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ fast-apple-datapath = []
[dependencies]
libc = "0.2.158"
log = { workspace = true, optional = true }
socket2 = { workspace = true }
tracing = { workspace = true, optional = true }

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

[target.'cfg(windows)'.dependencies]
once_cell = { workspace = true }
windows-sys = { workspace = true }
Expand All @@ -33,7 +35,7 @@ criterion = { version = "0.5", default-features = false, features = ["async_toki
tokio = { workspace = true, features = ["rt", "rt-multi-thread", "net"] }

[build-dependencies]
cfg_aliases = "0.2"
cfg_aliases = { workspace = true }

[lib]
# See https://github.com/bheisler/criterion.rs/blob/master/book/src/faq.md#cargo-bench-gives-unrecognized-option-errors-for-valid-command-line-options
Expand Down
1 change: 1 addition & 0 deletions quinn-udp/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,6 @@ fn main() {
// Convenience aliases
apple_fast: { all(apple, feature = "fast-apple-datapath") },
apple_slow: { all(apple, not(feature = "fast-apple-datapath")) },
wasm_browser: { all(target_family = "wasm", target_os = "unknown") },
}
}
16 changes: 12 additions & 4 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The newest version that avoids the feature in favor of a cfg alias wasm_browser makes lots of these cfgs simpler, as that cfg is exclusive with unix and windows cfgs.
Please take a look at the latest diff @mxinden I think you'll like it more.

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

use std::net::{IpAddr, Ipv6Addr, SocketAddr};
djc marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(unix)]
use std::os::unix::io::AsFd;
#[cfg(windows)]
use std::os::windows::io::AsSocket;
#[cfg(not(wasm_browser))]
use std::{
net::{IpAddr, Ipv6Addr, SocketAddr},
sync::Mutex,
time::{Duration, Instant},
};
Expand All @@ -49,7 +50,7 @@ mod imp;
mod imp;

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

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

#[cfg(not(wasm_browser))]
pub use imp::UdpSocketState;

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

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

/// Log at most 1 IO error per minute
#[cfg(not(wasm_browser))]
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(not(wasm_browser), any(feature = "tracing", feature = "direct-log")))]
fn log_sendmsg_error(
last_send_error: &Mutex<Instant>,
err: impl core::fmt::Debug,
Expand All @@ -164,14 +171,15 @@ fn log_sendmsg_error(
}

// No-op
#[cfg(not(any(feature = "tracing", feature = "direct-log")))]
#[cfg(not(any(wasm_browser, 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(not(wasm_browser))]
pub struct UdpSockRef<'a>(socket2::SockRef<'a>);

#[cfg(unix)]
Expand Down
13 changes: 11 additions & 2 deletions quinn/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ rustls-aws-lc-rs = ["dep:rustls", "aws-lc-rs", "proto/rustls-aws-lc-rs", "proto/
rustls-aws-lc-rs-fips = ["dep:rustls", "aws-lc-rs-fips", "proto/rustls-aws-lc-rs-fips", "proto/aws-lc-rs-fips"]
# Enable rustls with the `ring` crypto provider
rustls-ring = ["dep:rustls", "ring", "proto/rustls-ring", "proto/ring"]
# Enables `Endpoint::client` and `Endpoint::server` conveniences
# Enable the `ring` crypto provider.
# Outside wasm*-unknown-unknown targets, this 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"]
Expand All @@ -49,12 +50,17 @@ 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 }
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(not(all(target_family = "wasm", target_os = "unknown")))'.dependencies]
socket2 = { workspace = true }

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

[dev-dependencies]
anyhow = { workspace = true }
crc = { workspace = true }
Expand All @@ -69,6 +75,9 @@ tracing-subscriber = { workspace = true }
tracing-futures = { workspace = true }
url = { workspace = true }

[build-dependencies]
cfg_aliases = { workspace = true }

[[example]]
name = "server"
required-features = ["rustls-ring"]
Expand Down
9 changes: 9 additions & 0 deletions quinn/build.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use cfg_aliases::cfg_aliases;

fn main() {
// Setup cfg aliases
cfg_aliases! {
// Convenience aliases
wasm_browser: { all(target_family = "wasm", target_os = "unknown") },
}
}
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 @@ -10,14 +10,13 @@ use std::{
str,
sync::{Arc, Mutex},
task::{Context, Poll, Waker},
time::Instant,
};

#[cfg(any(feature = "aws-lc-rs", feature = "ring"))]
#[cfg(all(not(wasm_browser), 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 @@ -26,7 +25,7 @@ use proto::{
EndpointEvent, ServerConfig,
};
use rustc_hash::FxHashMap;
#[cfg(any(feature = "aws-lc-rs", feature = "ring"))]
#[cfg(all(not(wasm_browser), 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 @@ -68,7 +67,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(not(wasm_browser), 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 @@ -98,7 +97,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(not(wasm_browser), 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 @@ -112,6 +111,7 @@ impl Endpoint {
}

/// Construct an endpoint with arbitrary configuration and socket
#[cfg(not(wasm_browser))]
pub fn new(
config: EndpointConfig,
server_config: Option<ServerConfig>,
Expand Down Expand Up @@ -235,6 +235,7 @@ impl Endpoint {
/// Switch to a new UDP socket
///
/// See [`Endpoint::rebind_abstract()`] for details.
#[cfg(not(wasm_browser))]
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(wasm_browser))]
pub(crate) use std::time::{Duration, Instant};
#[cfg(wasm_browser)]
pub(crate) use web_time::{Duration, Instant};

pub use proto::{
congestion, crypto, AckFrequencyConfig, ApplicationClose, Chunk, ClientConfig, ClosedStream,
ConfigError, ConnectError, ConnectionClose, ConnectionError, ConnectionId,
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(not(wasm_browser))]
fn wrap_udp_socket(&self, t: std::net::UdpSocket) -> io::Result<Arc<dyn AsyncUdpSocket>>;
/// Look up the current time
///
Expand Down
8 changes: 3 additions & 5 deletions quinn/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,15 @@ use std::{
};

use crate::runtime::TokioRuntime;
use crate::{Duration, Instant};
use bytes::Bytes;
use proto::{crypto::rustls::QuicClientConfig, RandomConnectionIdGenerator};
use rand::{rngs::StdRng, RngCore, SeedableRng};
use rustls::{
pki_types::{CertificateDer, PrivateKeyDer, PrivatePkcs8KeyDer},
RootCertStore,
};
use tokio::{
runtime::{Builder, Runtime},
time::{Duration, Instant},
};
use tokio::runtime::{Builder, Runtime};
use tracing::{error_span, info};
use tracing_futures::Instrument as _;
use tracing_subscriber::EnvFilter;
Expand Down Expand Up @@ -160,7 +158,7 @@ fn read_after_close() {
.unwrap()
.await
.expect("connect");
tokio::time::sleep_until(Instant::now() + Duration::from_millis(100)).await;
tokio::time::sleep(Duration::from_millis(100)).await;
matheus23 marked this conversation as resolved.
Show resolved Hide resolved
let mut stream = new_conn.accept_uni().await.expect("incoming streams");
let msg = stream.read_to_end(usize::MAX).await.expect("read_to_end");
assert_eq!(msg, MSG);
Expand Down
2 changes: 1 addition & 1 deletion quinn/src/work_limiter.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::time::{Duration, Instant};
use crate::{Duration, Instant};

/// Limits the amount of time spent on a certain type of work in a cycle
///
Expand Down
Loading