From c9f5a363ad4b1f22c099319774dd0b8f1ef58d59 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 10 Jan 2025 10:53:22 +0200 Subject: [PATCH 1/6] ci: Unpin nightly (#2338) https://github.com/rust-lang/rust/issues/135235 is fixed --- .github/actions/rust/action.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/actions/rust/action.yml b/.github/actions/rust/action.yml index 7640e986a7..54fe0e45a1 100644 --- a/.github/actions/rust/action.yml +++ b/.github/actions/rust/action.yml @@ -24,8 +24,7 @@ runs: - name: Install Rust uses: dtolnay/rust-toolchain@21dc36fb71dd22e3317045c0c31a3f4249868b17 # master with: - # TODO: Unpin once https://github.com/rust-lang/rust/issues/135235 is fixed. - toolchain: ${{ inputs.version == 'nightly' && 'nightly-2025-01-07' || inputs.version }} + toolchain: ${{ inputs.version }} components: ${{ inputs.components }} targets: ${{ inputs.targets }} From ac2985e4e0e423f0be1038ef81086f7ae0beb6e0 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 10 Jan 2025 10:13:24 +0100 Subject: [PATCH 2/6] tests(udp): use blocking socket to receive empty datagram (#2339) The `handle_empty_datagram` unit tests sends and receives an empty datagram. To prevent a race condition where the receiver tries to receive the datagram before it is available (error `WouldBlock`), use a blocking socket via `socket()` instead. --- neqo-udp/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-udp/src/lib.rs b/neqo-udp/src/lib.rs index ef57a996f8..a68367aa8f 100644 --- a/neqo-udp/src/lib.rs +++ b/neqo-udp/src/lib.rs @@ -233,7 +233,7 @@ mod tests { // platforms. Use `std` socket instead. See also // . let sender = std::net::UdpSocket::bind("127.0.0.1:0")?; - let receiver = Socket::new(std::net::UdpSocket::bind("127.0.0.1:0")?)?; + let receiver = socket()?; let receiver_addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); sender.send_to(&[], receiver.inner.local_addr()?)?; From 80b4c3f27ba67cabc3621e37e50438024520a4be Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 10 Jan 2025 11:20:36 +0200 Subject: [PATCH 3/6] chore: Enable clippy `clone_on_ref_ptr` lint (#2314) * chore: Enable clippy `clone_on_ref_ptr` lint https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_ref_ptr * `#[allow(clippy::too_many_lines)]` --------- Signed-off-by: Lars Eggert --- Cargo.toml | 1 + neqo-http3/src/connection.rs | 10 +++---- neqo-http3/src/connection_server.rs | 2 +- .../extended_connect/webtransport_session.rs | 10 +++---- neqo-http3/src/server.rs | 29 ++++++++++++------- neqo-http3/src/server_events.rs | 2 +- neqo-transport/src/connection/mod.rs | 4 +-- test-fixture/src/sim/mod.rs | 2 +- 8 files changed, 35 insertions(+), 25 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 90e22c2c2b..d8cf1854d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,6 +59,7 @@ unused_macro_rules = "warn" cargo = { level = "warn", priority = -1 } nursery = { level = "warn", priority = -1 } pedantic = { level = "warn", priority = -1 } +clone_on_ref_ptr = "warn" if_then_some_else_none = "warn" get_unwrap = "warn" multiple_inherent_impl = "warn" diff --git a/neqo-http3/src/connection.rs b/neqo-http3/src/connection.rs index d3dd12d8fe..87efda041a 100644 --- a/neqo-http3/src/connection.rs +++ b/neqo-http3/src/connection.rs @@ -902,7 +902,7 @@ impl Http3Connection { MessageType::Request, stream_type, stream_id, - self.qpack_encoder.clone(), + Rc::clone(&self.qpack_encoder), send_events, ); @@ -1133,8 +1133,8 @@ impl Http3Connection { ))); self.add_streams( id, - Box::new(extended_conn.clone()), - Box::new(extended_conn.clone()), + Box::new(Rc::clone(&extended_conn)), + Box::new(Rc::clone(&extended_conn)), ); let final_headers = Self::create_fetch_headers(&RequestDescription { @@ -1217,7 +1217,7 @@ impl Http3Connection { ))); self.add_streams( stream_id, - Box::new(extended_conn.clone()), + Box::new(Rc::clone(&extended_conn)), Box::new(extended_conn), ); self.streams_with_pending_data.insert(stream_id); @@ -1368,7 +1368,7 @@ impl Http3Connection { stream_id, session_id, send_events, - webtransport_session.clone(), + Rc::clone(&webtransport_session), local, )), Box::new(WebTransportRecvStream::new( diff --git a/neqo-http3/src/connection_server.rs b/neqo-http3/src/connection_server.rs index c4e9b353c0..80e318f584 100644 --- a/neqo-http3/src/connection_server.rs +++ b/neqo-http3/src/connection_server.rs @@ -325,7 +325,7 @@ impl Http3ServerHandler { MessageType::Response, Http3StreamType::Http, stream_id, - self.base_handler.qpack_encoder.clone(), + Rc::clone(&self.base_handler.qpack_encoder), Box::new(self.events.clone()), )), Box::new(RecvMessage::new( diff --git a/neqo-http3/src/features/extended_connect/webtransport_session.rs b/neqo-http3/src/features/extended_connect/webtransport_session.rs index ea8a87659c..88b948e0d6 100644 --- a/neqo-http3/src/features/extended_connect/webtransport_session.rs +++ b/neqo-http3/src/features/extended_connect/webtransport_session.rs @@ -73,7 +73,7 @@ impl WebTransportSession { first_frame_type: None, }, qpack_decoder, - Box::new(stream_event_listener.clone()), + Box::new(Rc::clone(&stream_event_listener)), None, PriorityHandler::new(false, Priority::default()), )), @@ -82,7 +82,7 @@ impl WebTransportSession { Http3StreamType::ExtendedConnect, session_id, qpack_encoder, - Box::new(stream_event_listener.clone()), + Box::new(Rc::clone(&stream_event_listener)), )), stream_event_listener, session_id, @@ -111,11 +111,11 @@ impl WebTransportSession { control_stream_recv .http_stream() .unwrap() - .set_new_listener(Box::new(stream_event_listener.clone())); + .set_new_listener(Box::new(Rc::clone(&stream_event_listener))); control_stream_send .http_stream() .unwrap() - .set_new_listener(Box::new(stream_event_listener.clone())); + .set_new_listener(Box::new(Rc::clone(&stream_event_listener))); Self { control_stream_recv, control_stream_send, @@ -451,7 +451,7 @@ impl RecvStream for Rc> { } fn webtransport(&self) -> Option>> { - Some(self.clone()) + Some(Self::clone(self)) } } diff --git a/neqo-http3/src/server.rs b/neqo-http3/src/server.rs index 8f431f8b5f..c5914e2f60 100644 --- a/neqo-http3/src/server.rs +++ b/neqo-http3/src/server.rs @@ -152,6 +152,7 @@ impl Http3Server { } } + #[allow(clippy::too_many_lines)] fn process_events(&mut self, conn: &ConnectionRef, now: Instant) { let mut remove = false; let http3_parameters = &self.http3_parameters; @@ -172,7 +173,11 @@ impl Http3Server { headers, fin, } => self.events.headers( - Http3OrWebTransportStream::new(conn.clone(), handler.clone(), stream_info), + Http3OrWebTransportStream::new( + conn.clone(), + Rc::clone(handler), + stream_info, + ), headers, fin, ), @@ -188,15 +193,19 @@ impl Http3Server { } Http3ServerConnEvent::DataWritable { stream_info } => self .events - .data_writable(conn.clone(), handler.clone(), stream_info), + .data_writable(conn.clone(), Rc::clone(handler), stream_info), Http3ServerConnEvent::StreamReset { stream_info, error } => { - self.events - .stream_reset(conn.clone(), handler.clone(), stream_info, error); + self.events.stream_reset( + conn.clone(), + Rc::clone(handler), + stream_info, + error, + ); } Http3ServerConnEvent::StreamStopSending { stream_info, error } => { self.events.stream_stop_sending( conn.clone(), - handler.clone(), + Rc::clone(handler), stream_info, error, ); @@ -216,7 +225,7 @@ impl Http3Server { } Http3ServerConnEvent::ExtendedConnect { stream_id, headers } => { self.events.webtransport_new_session( - WebTransportRequest::new(conn.clone(), handler.clone(), stream_id), + WebTransportRequest::new(conn.clone(), Rc::clone(handler), stream_id), headers, ); } @@ -226,7 +235,7 @@ impl Http3Server { headers, .. } => self.events.webtransport_session_closed( - WebTransportRequest::new(conn.clone(), handler.clone(), stream_id), + WebTransportRequest::new(conn.clone(), Rc::clone(handler), stream_id), reason, headers, ), @@ -234,14 +243,14 @@ impl Http3Server { .events .webtransport_new_stream(Http3OrWebTransportStream::new( conn.clone(), - handler.clone(), + Rc::clone(handler), stream_info, )), Http3ServerConnEvent::ExtendedConnectDatagram { session_id, datagram, } => self.events.webtransport_datagram( - WebTransportRequest::new(conn.clone(), handler.clone(), session_id), + WebTransportRequest::new(conn.clone(), Rc::clone(handler), session_id), datagram, ), } @@ -294,7 +303,7 @@ fn prepare_data( data.resize(amount, 0); } - events.data(conn.clone(), handler.clone(), stream_info, data, fin); + events.data(conn.clone(), Rc::clone(handler), stream_info, data, fin); } if amount < MAX_EVENT_DATA_SIZE || fin { break; diff --git a/neqo-http3/src/server_events.rs b/neqo-http3/src/server_events.rs index 3f174ba0a0..87f30f85fc 100644 --- a/neqo-http3/src/server_events.rs +++ b/neqo-http3/src/server_events.rs @@ -335,7 +335,7 @@ impl WebTransportRequest { Ok(Http3OrWebTransportStream::new( self.stream_handler.conn.clone(), - self.stream_handler.handler.clone(), + Rc::clone(&self.stream_handler.handler), Http3StreamInfo::new(id, Http3StreamType::WebTransport(session_id)), )) } diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index d5943b40c5..d228116b69 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -406,7 +406,7 @@ impl Connection { state: State::Init, paths: Paths::default(), cid_manager, - tps: tphandler.clone(), + tps: Rc::clone(&tphandler), zero_rtt_state: ZeroRttState::Init, address_validation: AddressValidationInfo::None, local_initial_source_cid, @@ -444,7 +444,7 @@ impl Connection { zero_rtt_checker: impl ZeroRttChecker + 'static, ) -> Res<()> { self.crypto - .server_enable_0rtt(self.tps.clone(), anti_replay, zero_rtt_checker) + .server_enable_0rtt(Rc::clone(&self.tps), anti_replay, zero_rtt_checker) } /// # Errors diff --git a/test-fixture/src/sim/mod.rs b/test-fixture/src/sim/mod.rs index 5969d0b282..2d2c6e2e0d 100644 --- a/test-fixture/src/sim/mod.rs +++ b/test-fixture/src/sim/mod.rs @@ -249,7 +249,7 @@ impl Simulator { qinfo!("{}: seed {}", self.name, self.rng.borrow().seed_str()); for n in &mut self.nodes { - n.init(self.rng.clone(), start); + n.init(Rc::clone(&self.rng), start); } let setup_start = Instant::now(); From 1596bc6644803b199070fe1d12fde7dfeb480960 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 10 Jan 2025 11:46:37 +0200 Subject: [PATCH 4/6] chore: Enable the clippy `create_dir` lint (#2315) https://rust-lang.github.io/rust-clippy/master/index.html#create_dir Signed-off-by: Lars Eggert --- Cargo.toml | 1 + neqo-bin/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index d8cf1854d8..9e4bd326d8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,7 @@ cargo = { level = "warn", priority = -1 } nursery = { level = "warn", priority = -1 } pedantic = { level = "warn", priority = -1 } clone_on_ref_ptr = "warn" +create_dir = "warn" if_then_some_else_none = "warn" get_unwrap = "warn" multiple_inherent_impl = "warn" diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index 029d0086c6..122246be61 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -283,7 +283,7 @@ mod tests { .unwrap() .as_secs() )); - fs::create_dir(&dir).unwrap(); + fs::create_dir_all(&dir).unwrap(); Self { path: dir } } From 648863b3fdba6d08f9246151a1ec31171fd1bb0f Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 10 Jan 2025 12:24:33 +0200 Subject: [PATCH 5/6] chore: Enable clippy `cfg_not_test` (#2313) * chore: Enable clippy `cfg_not_test` https://rust-lang.github.io/rust-clippy/master/index.html#cfg_not_test * Fix * Remove `INTERNAL_TRANSPORT_PARAMETERS` --------- Signed-off-by: Lars Eggert --- Cargo.toml | 1 + neqo-transport/src/connection/mod.rs | 16 +++++++--------- neqo-transport/src/tparams.rs | 4 ---- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9e4bd326d8..fb6fdbddbc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,6 +59,7 @@ unused_macro_rules = "warn" cargo = { level = "warn", priority = -1 } nursery = { level = "warn", priority = -1 } pedantic = { level = "warn", priority = -1 } +cfg_not_test = "warn" clone_on_ref_ptr = "warn" create_dir = "warn" if_then_some_else_none = "warn" diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index d228116b69..666db13381 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -55,10 +55,7 @@ use crate::{ stats::{Stats, StatsCell}, stream_id::StreamType, streams::{SendOrder, Streams}, - tparams::{ - self, TransportParameter, TransportParameterId, TransportParameters, - TransportParametersHandler, - }, + tparams::{self, TransportParameters, TransportParametersHandler}, tracking::{AckTracker, PacketNumberSpace, RecvdPackets}, version::{Version, WireVersion}, AppError, CloseReason, Error, Res, StreamId, @@ -499,11 +496,12 @@ impl Connection { /// When the transport parameter is invalid. /// # Panics /// This panics if the transport parameter is known to this crate. - pub fn set_local_tparam(&self, tp: TransportParameterId, value: TransportParameter) -> Res<()> { - #[cfg(not(test))] - { - assert!(!tparams::INTERNAL_TRANSPORT_PARAMETERS.contains(&tp)); - } + #[cfg(test)] + pub fn set_local_tparam( + &self, + tp: tparams::TransportParameterId, + value: tparams::TransportParameter, + ) -> Res<()> { if *self.state() == State::Init { self.tps.borrow_mut().local.set(tp, value); Ok(()) diff --git a/neqo-transport/src/tparams.rs b/neqo-transport/src/tparams.rs index 2ecfb5b3a6..a9f30ef9fe 100644 --- a/neqo-transport/src/tparams.rs +++ b/neqo-transport/src/tparams.rs @@ -31,10 +31,6 @@ pub type TransportParameterId = u64; macro_rules! tpids { { $($n:ident = $v:expr),+ $(,)? } => { $(pub const $n: TransportParameterId = $v;)+ - - /// A complete list of internal transport parameters. - #[cfg(not(test))] - pub(crate) const INTERNAL_TRANSPORT_PARAMETERS: &[TransportParameterId] = &[ $($n),+ ]; }; } tpids! { From e66428087c882bd70cd89196348bf3fb470eb7d6 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 10 Jan 2025 14:57:33 +0200 Subject: [PATCH 6/6] chore: Enable the `unused_result_ok` lint (#2333) * chore: Enable the `unused_result_ok` lint This also exposed an unneeded `Result` being returned from `stream_fairness`, which this PR also removes. * Restore error handling around `stream_fairness` * Fix * Remove old comment --------- Signed-off-by: Lars Eggert --- Cargo.toml | 1 + neqo-http3/src/connection.rs | 11 +++++++---- neqo-http3/src/connection_client.rs | 4 ---- neqo-transport/src/connection/mod.rs | 2 +- neqo-transport/src/connection/tests/stream.rs | 4 ++-- neqo-transport/tests/connection.rs | 4 ++-- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fb6fdbddbc..26ddffb476 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -72,6 +72,7 @@ renamed_function_params = "warn" semicolon_inside_block = "warn" try_err = "warn" unneeded_field_pattern = "warn" +unused_result_ok = "warn" unused_trait_names = "warn" # Optimize build dependencies, because bindgen and proc macros / style diff --git a/neqo-http3/src/connection.rs b/neqo-http3/src/connection.rs index 87efda041a..b99613afd4 100644 --- a/neqo-http3/src/connection.rs +++ b/neqo-http3/src/connection.rs @@ -737,8 +737,12 @@ impl Http3Connection { conn.stream_stop_sending(stream_id, Error::HttpStreamCreation.code())?; return Ok(ReceiveOutput::NoOutput); } - // set incoming WebTransport streams to be fair (share bandwidth) - conn.stream_fairness(stream_id, true).ok(); + // Set incoming WebTransport streams to be fair (share bandwidth). + // We may call this with an invalid stream ID, so ignore that error. + match conn.stream_fairness(stream_id, true) { + Ok(()) | Err(neqo_transport::Error::InvalidStreamId) => (), + Err(e) => return Err(Error::from(e)), + }; qinfo!( [self], "A new WebTransport stream {} for session {}.", @@ -1283,8 +1287,7 @@ impl Http3Connection { .stream_create(stream_type) .map_err(|e| Error::map_stream_create_errors(&e))?; // Set outgoing WebTransport streams to be fair (share bandwidth) - // This really can't fail, panics if it does - conn.stream_fairness(stream_id, true).unwrap(); + conn.stream_fairness(stream_id, true)?; self.webtransport_create_stream_internal( wt, diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index d90c5be991..76d7d5fcd1 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -812,10 +812,6 @@ impl Http3Client { /// # Errors /// /// It may return `InvalidStreamId` if a stream does not exist anymore. - /// - /// # Panics - /// - /// This cannot panic. pub fn webtransport_set_fairness(&mut self, stream_id: StreamId, fairness: bool) -> Res<()> { Http3Connection::stream_set_fairness(&mut self.conn, stream_id, fairness) } diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 666db13381..b5dd19febd 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1557,7 +1557,7 @@ impl Connection { ); path.borrow_mut().add_received(d.len()); let res = self.input_path(&path, d, received); - self.capture_error(Some(path), now, 0, res).ok(); + _ = self.capture_error(Some(path), now, 0, res); } fn input_path( diff --git a/neqo-transport/src/connection/tests/stream.rs b/neqo-transport/src/connection/tests/stream.rs index b900c50382..166072d307 100644 --- a/neqo-transport/src/connection/tests/stream.rs +++ b/neqo-transport/src/connection/tests/stream.rs @@ -131,8 +131,8 @@ fn sendorder_test(order_of_sendorder: &[Option]) { streams.push(id); ordered.push((id, *sendorder)); // must be set before sendorder - client.streams.set_fairness(id, true).ok(); - client.streams.set_sendorder(id, *sendorder).ok(); + client.streams.set_fairness(id, true).unwrap(); + client.streams.set_sendorder(id, *sendorder).unwrap(); } // Write some data to all the streams for stream_id in streams { diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index fcbcf228e5..12b4829732 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -283,8 +283,8 @@ fn handshake_mlkem768x25519() { client .set_groups(&[neqo_crypto::TLS_GRP_KEM_MLKEM768X25519]) - .ok(); - client.send_additional_key_shares(0).ok(); + .unwrap(); + client.send_additional_key_shares(0).unwrap(); test_fixture::handshake(&mut client, &mut server); assert_eq!(*client.state(), State::Confirmed);