From 14cafbaa7fa88434def2c1d19e932c08e00173f8 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 08:40:04 +0300 Subject: [PATCH 01/13] fix: Include an ACK with a CONNECTION_CLOSE (#1854) * fix: Include an ACK with a CONNECTION_CLOSE Fixes #1161 * Address review comments * Send ACK before CC if there is space * Address code review * Address more review comments * Update neqo-transport/src/tracking.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Update neqo-transport/src/tracking.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Address code review --------- Signed-off-by: Lars Eggert Co-authored-by: Martin Thomson --- neqo-transport/src/connection/mod.rs | 48 +++++++++++++++----- neqo-transport/src/connection/state.rs | 9 ++-- neqo-transport/src/connection/tests/close.rs | 14 ++++++ neqo-transport/src/tracking.rs | 17 ++++--- 4 files changed, 67 insertions(+), 21 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index b8f598e4fc..a7c88e4019 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -56,7 +56,7 @@ use crate::{ self, TransportParameter, TransportParameterId, TransportParameters, TransportParametersHandler, }, - tracking::{AckTracker, PacketNumberSpace, SentPacket}, + tracking::{AckTracker, PacketNumberSpace, RecvdPackets, SentPacket}, version::{Version, WireVersion}, AppError, ConnectionError, Error, Res, StreamId, }; @@ -2189,6 +2189,40 @@ impl Connection { (tokens, ack_eliciting, padded) } + fn write_closing_frames( + &mut self, + close: &ClosingFrame, + builder: &mut PacketBuilder, + space: PacketNumberSpace, + now: Instant, + path: &PathRef, + tokens: &mut Vec, + ) { + if builder.remaining() > ClosingFrame::MIN_LENGTH + RecvdPackets::USEFUL_ACK_LEN { + // Include an ACK frame with the CONNECTION_CLOSE. + let limit = builder.limit(); + builder.set_limit(limit - ClosingFrame::MIN_LENGTH); + self.acks.immediate_ack(now); + self.acks.write_frame( + space, + now, + path.borrow().rtt().estimate(), + builder, + tokens, + &mut self.stats.borrow_mut().frame_tx, + ); + builder.set_limit(limit); + } + // ConnectionError::Application is only allowed at 1RTT. + let sanitized = if space == PacketNumberSpace::ApplicationData { + None + } else { + close.sanitize() + }; + sanitized.as_ref().unwrap_or(close).write_frame(builder); + self.stats.borrow_mut().frame_tx.connection_close += 1; + } + /// Build a datagram, possibly from multiple packets (for different PN /// spaces) and each containing 1+ frames. #[allow(clippy::too_many_lines)] // Yeah, that's just the way it is. @@ -2252,17 +2286,7 @@ impl Connection { let payload_start = builder.len(); let (mut tokens, mut ack_eliciting, mut padded) = (Vec::new(), false, false); if let Some(ref close) = closing_frame { - // ConnectionError::Application is only allowed at 1RTT. - let sanitized = if *space == PacketNumberSpace::ApplicationData { - None - } else { - close.sanitize() - }; - sanitized - .as_ref() - .unwrap_or(close) - .write_frame(&mut builder); - self.stats.borrow_mut().frame_tx.connection_close += 1; + self.write_closing_frames(close, &mut builder, *space, now, path, &mut tokens); } else { (tokens, ack_eliciting, padded) = self.write_frames(path, *space, &profile, &mut builder, now); diff --git a/neqo-transport/src/connection/state.rs b/neqo-transport/src/connection/state.rs index cc2f6e30d2..ecf91abd07 100644 --- a/neqo-transport/src/connection/state.rs +++ b/neqo-transport/src/connection/state.rs @@ -156,10 +156,13 @@ impl ClosingFrame { } } + /// Length of a closing frame with a truncated `reason_length`. Allow 8 bytes for the reason + /// phrase to ensure that if it needs to be truncated there is still at least a few bytes of + /// the value. + pub const MIN_LENGTH: usize = 1 + 8 + 8 + 2 + 8; + pub fn write_frame(&self, builder: &mut PacketBuilder) { - // Allow 8 bytes for the reason phrase to ensure that if it needs to be - // truncated there is still at least a few bytes of the value. - if builder.remaining() < 1 + 8 + 8 + 2 + 8 { + if builder.remaining() < ClosingFrame::MIN_LENGTH { return; } match &self.error { diff --git a/neqo-transport/src/connection/tests/close.rs b/neqo-transport/src/connection/tests/close.rs index 5351dd0d5c..ba6e5548d1 100644 --- a/neqo-transport/src/connection/tests/close.rs +++ b/neqo-transport/src/connection/tests/close.rs @@ -40,7 +40,14 @@ fn connection_close() { client.close(now, 42, ""); + let stats_before = client.stats().frame_tx; let out = client.process(None, now); + let stats_after = client.stats().frame_tx; + assert_eq!( + stats_after.connection_close, + stats_before.connection_close + 1 + ); + assert_eq!(stats_after.ack, stats_before.ack + 1); server.process_input(&out.dgram().unwrap(), now); assert_draining(&server, &Error::PeerApplicationError(42)); @@ -57,7 +64,14 @@ fn connection_close_with_long_reason_string() { let long_reason = String::from_utf8([0x61; 2048].to_vec()).unwrap(); client.close(now, 42, long_reason); + let stats_before = client.stats().frame_tx; let out = client.process(None, now); + let stats_after = client.stats().frame_tx; + assert_eq!( + stats_after.connection_close, + stats_before.connection_close + 1 + ); + assert_eq!(stats_after.ack, stats_before.ack + 1); server.process_input(&out.dgram().unwrap(), now); assert_draining(&server, &Error::PeerApplicationError(42)); diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index d0723bbcbe..6643d516e3 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -559,6 +559,10 @@ impl RecvdPackets { } } + /// Length of the worst possible ACK frame, assuming only one range and ECN counts. + /// Note that this assumes one byte for the type and count of extra ranges. + pub const USEFUL_ACK_LEN: usize = 1 + 8 + 8 + 1 + 8 + 3 * 8; + /// Generate an ACK frame for this packet number space. /// /// Unlike other frame generators this doesn't modify the underlying instance @@ -577,10 +581,6 @@ impl RecvdPackets { tokens: &mut Vec, stats: &mut FrameStats, ) { - // The worst possible ACK frame, assuming only one range. - // Note that this assumes one byte for the type and count of extra ranges. - const LONGEST_ACK_HEADER: usize = 1 + 8 + 8 + 1 + 8; - // Check that we aren't delaying ACKs. if !self.ack_now(now, rtt) { return; @@ -592,7 +592,10 @@ impl RecvdPackets { // When congestion limited, ACK-only packets are 255 bytes at most // (`recovery::ACK_ONLY_SIZE_LIMIT - 1`). This results in limiting the // ranges to 13 here. - let max_ranges = if let Some(avail) = builder.remaining().checked_sub(LONGEST_ACK_HEADER) { + let max_ranges = if let Some(avail) = builder + .remaining() + .checked_sub(RecvdPackets::USEFUL_ACK_LEN) + { // Apply a hard maximum to keep plenty of space for other stuff. min(1 + (avail / 16), MAX_ACKS_PER_FRAME) } else { @@ -1158,7 +1161,9 @@ mod tests { .is_some()); let mut builder = PacketBuilder::short(Encoder::new(), false, []); - builder.set_limit(32); + // The code pessimistically assumes that each range needs 16 bytes to express. + // So this won't be enough for a second range. + builder.set_limit(RecvdPackets::USEFUL_ACK_LEN + 8); let mut stats = FrameStats::default(); tracker.write_frame( From 7070e7bcc321951552dab76022bc76cbe8a02f79 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 10:27:21 +0300 Subject: [PATCH 02/13] ci: Prepare to show failure differences of QNS run relative to main (#1856) * WIP * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Again * Make less noisy * Restore condition --------- Signed-off-by: Lars Eggert --- .github/workflows/qns.yml | 65 ++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 11 deletions(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 45ea31ffc5..604a28c93a 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -1,8 +1,8 @@ name: QUIC Network Simulator - on: schedule: - - cron: '42 3 * * 2,5' # Runs at 03:42 UTC (m and h chosen arbitrarily) twice a week. + # Run at 1 AM each day, so there is a `main`-branch baseline in the cache. + - cron: '0 1 * * *' workflow_dispatch: pull_request: branches: ["main"] @@ -78,7 +78,6 @@ jobs: path: /tmp/${{ env.LATEST }}.tar implementations: - if: ${{ github.event_name == 'pull_request' }} name: Determine interop pairs needs: docker-image runs-on: ubuntu-latest @@ -120,7 +119,6 @@ jobs: } >> "$GITHUB_OUTPUT" run-qns: - if: ${{ github.event_name == 'pull_request' }} name: Run QNS needs: implementations strategy: @@ -150,13 +148,20 @@ jobs: client: ${{ steps.depair.outputs.client }} server: ${{ steps.depair.outputs.server }} implementations: ${{ needs.implementations.outputs.implementations }} + test: handshake,transfer report: - if: ${{ always() && github.event_name == 'pull_request' }} name: Report results needs: run-qns runs-on: ubuntu-latest steps: + - name: Download cached main-branch results + uses: actions/cache/restore@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 + with: + path: results-main + key: qns-${{ runner.os }}-${{ github.sha }} + restore-keys: qns-${{ runner.os }}- + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 with: @@ -164,6 +169,9 @@ jobs: path: results - run: | + ls -l results-main || true + cat results-main/result.json || true + echo "[]" > result.json for RUN in results/*; do [ -e "$RUN/result.json" ] || continue CLIENT=$(jq -r < "$RUN/result.json" '.clients[0]') @@ -193,22 +201,57 @@ jobs: echo "**$RESULT**" } >> "$GROUP.md" done + jq < "$RUN/grouped.json" ". += { client: \"$CLIENT\", server: \"$SERVER\" }" > new.json && \ + jq < result.json --argjson new "$(cat new.json)" '. += [$new]' > result.json.tmp && \ + rm new.json && \ + mv result.json.tmp result.json done + DIFFER='def post_recurse(f): def r: (f | select(. != null) | r), .; r; def post_recurse: post_recurse(.[]?); (. | (post_recurse | arrays) |= sort)' + diff <(jq -S "$DIFFER" results-main/result.json) <(jq -S "$DIFFER" result.json) || true + diff -Baur results-main/result.json result.json || true { echo "### Failed Interop Tests" + SHA=$(cat results-main/baseline-sha.txt || true) + if [ -n "$SHA" ]; then + { + echo "Interop failures relative to $SHA." + echo + } >> results.md + fi + if [ -e failed.md ]; then + echo "[QUIC Interop Runner](https://github.com/quic-interop/quic-interop-runner), *client* vs. *server*" + cat failed.md + else + echo "None :tada:" + fi + echo "
All results" + echo echo "[QUIC Interop Runner](https://github.com/quic-interop/quic-interop-runner), *client* vs. *server*" - cat failed.md - echo "
Succeeded and unsupported tests" for GROUP in succeeded unsupported; do - echo echo "### ${GROUP^} Interop Tests" - echo - cat "$GROUP.md" - echo + if [ -e "$GROUP.md" ]; then + cat "$GROUP.md" + else + echo "None :question:" + fi done + echo echo "
" } >> comment.md + - name: Remember main-branch push URL + if: github.ref == 'refs/heads/main' + run: echo "${{ github.sha }}" > baseline-sha.txt + + - name: Cache main-branch results + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 + with: + path: | + result.json + baseline-sha.txt + key: qns-${{ runner.os }}-${{ github.sha }} + - uses: ./.github/actions/pr-comment-data-export with: name: qns From 8c4411a397ae8c3ab050888a342258c42864cc9c Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 13:24:43 +0300 Subject: [PATCH 03/13] chore: Bump MSRV to 1.76 (#1865) * chore: Bump MSRV to 1.76 Gecko did two days ago * Update check.yml --- .github/workflows/check.yml | 5 +++-- Cargo.toml | 3 ++- neqo-crypto/src/agentio.rs | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 00971a6ac4..99ba122d2d 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -25,8 +25,9 @@ jobs: matrix: os: [ubuntu-latest, macos-14, windows-latest] # Don't increase beyond what Firefox is currently using: - # https://firefox-source-docs.mozilla.org/writing-rust-code/update-policy.html#schedule - rust-toolchain: [1.74.0, stable, nightly] + # https://searchfox.org/mozilla-central/search?q=MINIMUM_RUST_VERSION&path=python/mozboot/mozboot/util.py + # Keep in sync with Cargo.toml + rust-toolchain: [1.76.0, stable, nightly] type: [debug] include: - os: ubuntu-latest diff --git a/Cargo.toml b/Cargo.toml index 017918b88c..4be3ba5ad4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,8 @@ edition = "2021" license = "MIT OR Apache-2.0" # Don't increase beyond what Firefox is currently using: # https://searchfox.org/mozilla-central/search?q=MINIMUM_RUST_VERSION&path=python/mozboot/mozboot/util.py -rust-version = "1.74.0" +# Keep in sync with .github/workflows/check.yml +rust-version = "1.76.0" [workspace.dependencies] log = { version = "0.4", default-features = false } diff --git a/neqo-crypto/src/agentio.rs b/neqo-crypto/src/agentio.rs index 7c57a0ef45..3beede5c12 100644 --- a/neqo-crypto/src/agentio.rs +++ b/neqo-crypto/src/agentio.rs @@ -29,7 +29,7 @@ const PR_FAILURE: PrStatus = prio::PRStatus::PR_FAILURE; /// Convert a pinned, boxed object into a void pointer. pub fn as_c_void(pin: &mut Pin>) -> *mut c_void { - (Pin::into_inner(pin.as_mut()) as *mut T).cast() + (std::ptr::from_mut::(Pin::into_inner(pin.as_mut()))).cast() } /// A slice of the output. From 8192300d974c09dcb3322cc7ee18e80f8a0af9f2 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 15:38:55 +0300 Subject: [PATCH 04/13] ci: More fixes to eventually generate a diff for QNS results (#1867) * ci: Generate a diff for QNS results WIP * Always store local docker image --- .github/workflows/qns.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 604a28c93a..9d5b9ffcec 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -60,7 +60,6 @@ jobs: platforms: 'linux/amd64, linux/arm64' - uses: docker/build-push-action@2cdde995de11925a030ce8070c3d77a52ffcf1c0 # v5.3.0 - if: github.event_name == 'pull_request' id: docker_build_and_push with: tags: ${{ steps.meta.outputs.tags }} @@ -72,7 +71,6 @@ jobs: outputs: type=docker,dest=/tmp/${{ env.LATEST }}.tar - uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 - if: github.event_name == 'pull_request' with: name: '${{ env.LATEST }} Docker image' path: /tmp/${{ env.LATEST }}.tar @@ -148,7 +146,6 @@ jobs: client: ${{ steps.depair.outputs.client }} server: ${{ steps.depair.outputs.server }} implementations: ${{ needs.implementations.outputs.implementations }} - test: handshake,transfer report: name: Report results @@ -226,10 +223,10 @@ jobs: fi echo "
All results" echo - echo "[QUIC Interop Runner](https://github.com/quic-interop/quic-interop-runner), *client* vs. *server*" for GROUP in succeeded unsupported; do echo "### ${GROUP^} Interop Tests" if [ -e "$GROUP.md" ]; then + echo "[QUIC Interop Runner](https://github.com/quic-interop/quic-interop-runner), *client* vs. *server*" cat "$GROUP.md" else echo "None :question:" From 620244dd9d090a1c99bfba0ac9c6f037e4ba93ac Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 15:37:22 +0300 Subject: [PATCH 05/13] feat: Make `reason_phrase` a String (#1862) * feat: Make `reason_phrase` a String To make the debug logs a bit easier to parse. * Fix clippy * Update neqo-transport/src/frame.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Update neqo-transport/src/qlog.rs Co-authored-by: Max Inden Signed-off-by: Lars Eggert * Address code review --------- Signed-off-by: Lars Eggert Co-authored-by: Martin Thomson Co-authored-by: Max Inden --- neqo-transport/src/connection/mod.rs | 1 - neqo-transport/src/frame.rs | 8 ++--- neqo-transport/src/qlog.rs | 48 ++++++++++++++-------------- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index a7c88e4019..632d7fc866 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2828,7 +2828,6 @@ impl Connection { reason_phrase, } => { self.stats.borrow_mut().frame_rx.connection_close += 1; - let reason_phrase = String::from_utf8_lossy(&reason_phrase); qinfo!( [self], "ConnectionClose received. Error code: {:?} frame type {:x} reason {}", diff --git a/neqo-transport/src/frame.rs b/neqo-transport/src/frame.rs index eba7009d4b..279bfa5c25 100644 --- a/neqo-transport/src/frame.rs +++ b/neqo-transport/src/frame.rs @@ -184,7 +184,7 @@ pub enum Frame<'a> { frame_type: u64, // Not a reference as we use this to hold the value. // This is not used in optimized builds anyway. - reason_phrase: Vec, + reason_phrase: String, }, HandshakeDone, AckFrequency { @@ -614,7 +614,7 @@ impl<'a> Frame<'a> { 0 }; // We can tolerate this copy for now. - let reason_phrase = d(dec.decode_vvec())?.to_vec(); + let reason_phrase = String::from_utf8_lossy(d(dec.decode_vvec())?).to_string(); Ok(Self::ConnectionClose { error_code, frame_type, @@ -925,7 +925,7 @@ mod tests { let f = Frame::ConnectionClose { error_code: CloseError::Transport(0x5678), frame_type: 0x1234, - reason_phrase: vec![0x01, 0x02, 0x03], + reason_phrase: String::from("\x01\x02\x03"), }; just_dec(&f, "1c80005678523403010203"); @@ -936,7 +936,7 @@ mod tests { let f = Frame::ConnectionClose { error_code: CloseError::Application(0x5678), frame_type: 0, - reason_phrase: vec![0x01, 0x02, 0x03], + reason_phrase: String::from("\x01\x02\x03"), }; just_dec(&f, "1d8000567803010203"); diff --git a/neqo-transport/src/qlog.rs b/neqo-transport/src/qlog.rs index fa1d56815c..715ba85e81 100644 --- a/neqo-transport/src/qlog.rs +++ b/neqo-transport/src/qlog.rs @@ -205,7 +205,7 @@ pub fn packet_sent( let mut frames = SmallVec::new(); while d.remaining() > 0 { if let Ok(f) = Frame::decode(&mut d) { - frames.push(QuicFrame::from(&f)); + frames.push(QuicFrame::from(f)); } else { qinfo!("qlog: invalid frame"); break; @@ -293,7 +293,7 @@ pub fn packet_received( while d.remaining() > 0 { if let Ok(f) = Frame::decode(&mut d) { - frames.push(QuicFrame::from(&f)); + frames.push(QuicFrame::from(f)); } else { qinfo!("qlog: invalid frame"); break; @@ -387,12 +387,12 @@ pub fn metrics_updated(qlog: &mut NeqoQlog, updated_metrics: &[QlogMetric]) { #[allow(clippy::too_many_lines)] // Yeah, but it's a nice match. #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)] // No choice here. -impl From<&Frame<'_>> for QuicFrame { - fn from(frame: &Frame) -> Self { +impl From> for QuicFrame { + fn from(frame: Frame) -> Self { match frame { Frame::Padding(len) => QuicFrame::Padding { length: None, - payload_length: u32::from(*len), + payload_length: u32::from(len), }, Frame::Ping => QuicFrame::Ping { length: None, @@ -406,7 +406,7 @@ impl From<&Frame<'_>> for QuicFrame { ecn_count, } => { let ranges = - Frame::decode_ack_frame(*largest_acknowledged, *first_ack_range, ack_ranges) + Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges) .ok(); let acked_ranges = ranges.map(|all| { @@ -418,7 +418,7 @@ impl From<&Frame<'_>> for QuicFrame { }); QuicFrame::Ack { - ack_delay: Some(*ack_delay as f32 / 1000.0), + ack_delay: Some(ack_delay as f32 / 1000.0), acked_ranges, ect1: ecn_count.map(|c| c[IpTosEcn::Ect1]), ect0: ecn_count.map(|c| c[IpTosEcn::Ect0]), @@ -433,8 +433,8 @@ impl From<&Frame<'_>> for QuicFrame { final_size, } => QuicFrame::ResetStream { stream_id: stream_id.as_u64(), - error_code: *application_error_code, - final_size: *final_size, + error_code: application_error_code, + final_size, length: None, payload_length: None, }, @@ -443,12 +443,12 @@ impl From<&Frame<'_>> for QuicFrame { application_error_code, } => QuicFrame::StopSending { stream_id: stream_id.as_u64(), - error_code: *application_error_code, + error_code: application_error_code, length: None, payload_length: None, }, Frame::Crypto { offset, data } => QuicFrame::Crypto { - offset: *offset, + offset, length: data.len() as u64, }, Frame::NewToken { token } => QuicFrame::NewToken { @@ -470,20 +470,20 @@ impl From<&Frame<'_>> for QuicFrame { .. } => QuicFrame::Stream { stream_id: stream_id.as_u64(), - offset: *offset, + offset, length: data.len() as u64, - fin: Some(*fin), + fin: Some(fin), raw: None, }, Frame::MaxData { maximum_data } => QuicFrame::MaxData { - maximum: *maximum_data, + maximum: maximum_data, }, Frame::MaxStreamData { stream_id, maximum_stream_data, } => QuicFrame::MaxStreamData { stream_id: stream_id.as_u64(), - maximum: *maximum_stream_data, + maximum: maximum_stream_data, }, Frame::MaxStreams { stream_type, @@ -493,15 +493,15 @@ impl From<&Frame<'_>> for QuicFrame { NeqoStreamType::BiDi => StreamType::Bidirectional, NeqoStreamType::UniDi => StreamType::Unidirectional, }, - maximum: *maximum_streams, + maximum: maximum_streams, }, - Frame::DataBlocked { data_limit } => QuicFrame::DataBlocked { limit: *data_limit }, + Frame::DataBlocked { data_limit } => QuicFrame::DataBlocked { limit: data_limit }, Frame::StreamDataBlocked { stream_id, stream_data_limit, } => QuicFrame::StreamDataBlocked { stream_id: stream_id.as_u64(), - limit: *stream_data_limit, + limit: stream_data_limit, }, Frame::StreamsBlocked { stream_type, @@ -511,7 +511,7 @@ impl From<&Frame<'_>> for QuicFrame { NeqoStreamType::BiDi => StreamType::Bidirectional, NeqoStreamType::UniDi => StreamType::Unidirectional, }, - limit: *stream_limit, + limit: stream_limit, }, Frame::NewConnectionId { sequence_number, @@ -519,14 +519,14 @@ impl From<&Frame<'_>> for QuicFrame { connection_id, stateless_reset_token, } => QuicFrame::NewConnectionId { - sequence_number: *sequence_number as u32, - retire_prior_to: *retire_prior as u32, + sequence_number: sequence_number as u32, + retire_prior_to: retire_prior as u32, connection_id_length: Some(connection_id.len() as u8), connection_id: hex(connection_id), stateless_reset_token: Some(hex(stateless_reset_token)), }, Frame::RetireConnectionId { sequence_number } => QuicFrame::RetireConnectionId { - sequence_number: *sequence_number as u32, + sequence_number: sequence_number as u32, }, Frame::PathChallenge { data } => QuicFrame::PathChallenge { data: Some(hex(data)), @@ -545,8 +545,8 @@ impl From<&Frame<'_>> for QuicFrame { }, error_code: Some(error_code.code()), error_code_value: Some(0), - reason: Some(String::from_utf8_lossy(reason_phrase).to_string()), - trigger_frame_type: Some(*frame_type), + reason: Some(reason_phrase), + trigger_frame_type: Some(frame_type), }, Frame::HandshakeDone => QuicFrame::HandshakeDone, Frame::AckFrequency { .. } => QuicFrame::Unknown { From 065dc17fecaaadc41e8f58953fe64d64357e3960 Mon Sep 17 00:00:00 2001 From: Kershaw Date: Thu, 2 May 2024 15:09:35 +0200 Subject: [PATCH 06/13] neqo v0.7.6 (#1869) --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 4be3ba5ad4..9eb8a19244 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ resolver = "2" homepage = "https://github.com/mozilla/neqo/" repository = "https://github.com/mozilla/neqo/" authors = ["The Neqo Authors "] -version = "0.7.5" +version = "0.7.6" # Keep in sync with `.rustfmt.toml` `edition`. edition = "2021" license = "MIT OR Apache-2.0" From 0b498cdbbde7c129eb04c4e9494e73744a785ae8 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 17:14:37 +0300 Subject: [PATCH 07/13] ci: Need to run QNS on push to main (#1870) So we get baseline data --- .github/workflows/qns.yml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 9d5b9ffcec..904eab03c3 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -1,12 +1,16 @@ name: QUIC Network Simulator on: + push: + branches: ["main"] + paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"] + pull_request: + branches: ["main"] + paths-ignore: ["*.md", "*.png", "*.svg", "LICENSE-*"] + merge_group: schedule: # Run at 1 AM each day, so there is a `main`-branch baseline in the cache. - cron: '0 1 * * *' workflow_dispatch: - pull_request: - branches: ["main"] - merge_group: concurrency: group: ${{ github.workflow }}-${{ github.ref_name }} From 87bf852e7332f4e5ffa2e08bb772e71bcd783771 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 2 May 2024 16:16:25 +0200 Subject: [PATCH 08/13] fix(bin/client): don't close closing connection (#1866) * fix(bin/client): don't close closing connection The `bin/src/client/mod.rs` `Runner::run` function continuously checks whether there is more work. In case there is none, it initiates closing of the connection (`self.client.close`) and then `continue`s to the top of the loop in order to send out a closing frame. https://github.com/mozilla/neqo/blob/14cafbaa7fa88434def2c1d19e932c08e00173f8/neqo-bin/src/client/mod.rs#L376-L409 There is a potential busy loop when closing an already closing connection. `Runner::run` will call `self.client.close` and then continously `continue` to the top of the loop. This commit differentiates a connection state in `NotClosing`, `Closing` and `Closed`. It only attempts to close a `NotClosing` connection and only then `continue`s to the top of the loop. * Introduce ConnectionError::is_error and implement TryFrom --- neqo-bin/src/client/http09.rs | 33 +++++++++++++++++++++++---------- neqo-bin/src/client/http3.rs | 31 +++++++++++++++++++++---------- neqo-bin/src/client/mod.rs | 19 ++++++++++++------- neqo-transport/src/lib.rs | 10 ++++++++++ 4 files changed, 66 insertions(+), 27 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index e9de5915a7..3bd6701ac0 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -25,7 +25,7 @@ use neqo_transport::{ }; use url::Url; -use super::{get_output_file, qlog_new, Args, Res}; +use super::{get_output_file, qlog_new, Args, CloseState, Res}; pub struct Handler<'a> { streams: HashMap>>, @@ -142,6 +142,26 @@ pub(crate) fn create_client( Ok(client) } +impl TryFrom<&State> for CloseState { + type Error = ConnectionError; + + fn try_from(value: &State) -> Result { + let (state, error) = match value { + State::Closing { error, .. } | State::Draining { error, .. } => { + (CloseState::Closing, error) + } + State::Closed(error) => (CloseState::Closed, error), + _ => return Ok(CloseState::NotClosing), + }; + + if error.is_error() { + Err(error.clone()) + } else { + Ok(state) + } + } +} + impl super::Client for Connection { fn process_output(&mut self, now: Instant) -> Output { self.process_output(now) @@ -163,15 +183,8 @@ impl super::Client for Connection { } } - fn is_closed(&self) -> Result { - match self.state() { - State::Closed( - ConnectionError::Transport(neqo_transport::Error::NoError) - | ConnectionError::Application(0), - ) => Ok(true), - State::Closed(err) => Err(err.clone()), - _ => Ok(false), - } + fn is_closed(&self) -> Result { + self.state().try_into() } fn stats(&self) -> neqo_transport::Stats { diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 5a77c92f0b..0884f79720 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -27,7 +27,7 @@ use neqo_transport::{ }; use url::Url; -use super::{get_output_file, qlog_new, Args, Res}; +use super::{get_output_file, qlog_new, Args, CloseState, Res}; pub(crate) struct Handler<'a> { #[allow( @@ -105,17 +105,28 @@ pub(crate) fn create_client( Ok(client) } -impl super::Client for Http3Client { - fn is_closed(&self) -> Result { - match self.state() { - Http3State::Closed( - ConnectionError::Transport(neqo_transport::Error::NoError) - | ConnectionError::Application(0), - ) => Ok(true), - Http3State::Closed(err) => Err(err.clone()), - _ => Ok(false), +impl TryFrom for CloseState { + type Error = ConnectionError; + + fn try_from(value: Http3State) -> Result { + let (state, error) = match value { + Http3State::Closing(error) => (CloseState::Closing, error), + Http3State::Closed(error) => (CloseState::Closed, error), + _ => return Ok(CloseState::NotClosing), + }; + + if error.is_error() { + Err(error.clone()) + } else { + Ok(state) } } +} + +impl super::Client for Http3Client { + fn is_closed(&self) -> Result { + self.state().try_into() + } fn process_output(&mut self, now: Instant) -> Output { self.process_output(now) diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 6d0b5dad6f..6cbd3176dd 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -345,6 +345,12 @@ trait Handler { fn take_token(&mut self) -> Option; } +enum CloseState { + NotClosing, + Closing, + Closed, +} + /// Network client, e.g. [`neqo_transport::Connection`] or [`neqo_http3::Http3Client`]. trait Client { fn process_output(&mut self, now: Instant) -> Output; @@ -355,11 +361,7 @@ trait Client { fn close(&mut self, now: Instant, app_error: AppError, msg: S) where S: AsRef + Display; - /// Returns [`Some(_)`] if the connection is closed. - /// - /// Note that connection was closed without error on - /// [`Some(ConnectionError::Transport(TransportError::NoError))`]. - fn is_closed(&self) -> Result; + fn is_closed(&self) -> Result; fn stats(&self) -> neqo_transport::Stats; } @@ -381,16 +383,19 @@ impl<'a, H: Handler> Runner<'a, H> { continue; } + #[allow(clippy::match_same_arms)] match (handler_done, self.client.is_closed()?) { // more work (false, _) => {} // no more work, closing connection - (true, false) => { + (true, CloseState::NotClosing) => { self.client.close(Instant::now(), 0, "kthxbye!"); continue; } + // no more work, already closing connection + (true, CloseState::Closing) => {} // no more work, connection closed, terminating - (true, true) => break, + (true, CloseState::Closed) => break, } match ready(self.socket, self.timeout.as_mut()).await? { diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 53af334e27..f0d126569a 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -223,6 +223,16 @@ impl ConnectionError { Self::Transport(_) => None, } } + + /// Checks enclosed error for [`Error::NoError`] and + /// [`ConnectionError::Application(0)`]. + #[must_use] + pub fn is_error(&self) -> bool { + !matches!( + self, + ConnectionError::Transport(Error::NoError) | ConnectionError::Application(0), + ) + } } impl From for ConnectionError { From ed19eb229d0d80fdbd0e1581488abd9cd1a73be7 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 3 May 2024 10:03:45 +0200 Subject: [PATCH 09/13] refactor: rename ConnectionError to CloseReason (#1872) The `neqo_transport::ConnectionError` enum contains the two non-error variants `Error::NoError` and `CloseReason::Application(0)`. In other words, `ConnectionError` contains variants that are not errors. This commit renames `ConnectionError` to the more descriptive name `CloseReason`. See suggestion in https://github.com/mozilla/neqo/pull/1866#issuecomment-2091654649. To ease the upgrade for downstream users, this commit adds a deprecated `ConnectionError`, guiding users to rename to `CloseReason` via a deprecation warning. ``` rust pub type ConnectionError = CloseReason; ``` --- neqo-bin/src/client/http09.rs | 6 ++--- neqo-bin/src/client/http3.rs | 8 +++--- neqo-bin/src/client/mod.rs | 12 ++++----- neqo-http3/src/connection.rs | 20 +++++++------- neqo-http3/src/connection_client.rs | 20 +++++++------- .../tests/webtransport/negotiation.rs | 7 ++--- neqo-http3/src/server.rs | 4 +-- neqo-http3/tests/httpconn.rs | 4 +-- neqo-transport/src/connection/mod.rs | 18 ++++++------- neqo-transport/src/connection/state.rs | 26 +++++++++---------- neqo-transport/src/connection/tests/close.rs | 11 ++++---- .../src/connection/tests/datagram.rs | 12 +++------ .../src/connection/tests/handshake.rs | 26 ++++++++----------- neqo-transport/src/connection/tests/keys.rs | 10 +++---- .../src/connection/tests/migration.rs | 6 ++--- neqo-transport/src/connection/tests/mod.rs | 8 +++--- neqo-transport/src/connection/tests/stream.rs | 9 +++---- neqo-transport/src/connection/tests/vn.rs | 8 +++--- neqo-transport/src/events.rs | 4 +-- neqo-transport/src/frame.rs | 10 +++---- neqo-transport/src/lib.rs | 14 ++++++---- neqo-transport/tests/connection.rs | 9 +++---- neqo-transport/tests/network.rs | 10 +++---- neqo-transport/tests/retry.rs | 4 +-- neqo-transport/tests/server.rs | 6 ++--- 25 files changed, 127 insertions(+), 145 deletions(-) diff --git a/neqo-bin/src/client/http09.rs b/neqo-bin/src/client/http09.rs index 3bd6701ac0..964e09c822 100644 --- a/neqo-bin/src/client/http09.rs +++ b/neqo-bin/src/client/http09.rs @@ -20,7 +20,7 @@ use std::{ use neqo_common::{event::Provider, qdebug, qinfo, qwarn, Datagram}; use neqo_crypto::{AuthenticationStatus, ResumptionToken}; use neqo_transport::{ - Connection, ConnectionError, ConnectionEvent, EmptyConnectionIdGenerator, Error, Output, State, + CloseReason, Connection, ConnectionEvent, EmptyConnectionIdGenerator, Error, Output, State, StreamId, StreamType, }; use url::Url; @@ -143,7 +143,7 @@ pub(crate) fn create_client( } impl TryFrom<&State> for CloseState { - type Error = ConnectionError; + type Error = CloseReason; fn try_from(value: &State) -> Result { let (state, error) = match value { @@ -183,7 +183,7 @@ impl super::Client for Connection { } } - fn is_closed(&self) -> Result { + fn is_closed(&self) -> Result { self.state().try_into() } diff --git a/neqo-bin/src/client/http3.rs b/neqo-bin/src/client/http3.rs index 0884f79720..8284bd5d34 100644 --- a/neqo-bin/src/client/http3.rs +++ b/neqo-bin/src/client/http3.rs @@ -22,8 +22,8 @@ use neqo_common::{event::Provider, hex, qdebug, qinfo, qwarn, Datagram, Header}; use neqo_crypto::{AuthenticationStatus, ResumptionToken}; use neqo_http3::{Error, Http3Client, Http3ClientEvent, Http3Parameters, Http3State, Priority}; use neqo_transport::{ - AppError, Connection, ConnectionError, EmptyConnectionIdGenerator, Error as TransportError, - Output, StreamId, + AppError, CloseReason, Connection, EmptyConnectionIdGenerator, Error as TransportError, Output, + StreamId, }; use url::Url; @@ -106,7 +106,7 @@ pub(crate) fn create_client( } impl TryFrom for CloseState { - type Error = ConnectionError; + type Error = CloseReason; fn try_from(value: Http3State) -> Result { let (state, error) = match value { @@ -124,7 +124,7 @@ impl TryFrom for CloseState { } impl super::Client for Http3Client { - fn is_closed(&self) -> Result { + fn is_closed(&self) -> Result { self.state().try_into() } diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index 6cbd3176dd..f196a5e32e 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -27,7 +27,7 @@ use neqo_crypto::{ init, Cipher, ResumptionToken, }; use neqo_http3::Output; -use neqo_transport::{AppError, ConnectionError, ConnectionId, Version}; +use neqo_transport::{AppError, CloseReason, ConnectionId, Version}; use qlog::{events::EventImportance, streamer::QlogStreamer}; use tokio::time::Sleep; use url::{Origin, Url}; @@ -80,11 +80,11 @@ impl From for Error { } } -impl From for Error { - fn from(err: neqo_transport::ConnectionError) -> Self { +impl From for Error { + fn from(err: neqo_transport::CloseReason) -> Self { match err { - ConnectionError::Transport(e) => Self::TransportError(e), - ConnectionError::Application(e) => Self::ApplicationError(e), + CloseReason::Transport(e) => Self::TransportError(e), + CloseReason::Application(e) => Self::ApplicationError(e), } } } @@ -361,7 +361,7 @@ trait Client { fn close(&mut self, now: Instant, app_error: AppError, msg: S) where S: AsRef + Display; - fn is_closed(&self) -> Result; + fn is_closed(&self) -> Result; fn stats(&self) -> neqo_transport::Stats; } diff --git a/neqo-http3/src/connection.rs b/neqo-http3/src/connection.rs index dd45797baa..d14eb6f2a5 100644 --- a/neqo-http3/src/connection.rs +++ b/neqo-http3/src/connection.rs @@ -17,7 +17,7 @@ use std::{ use neqo_common::{qdebug, qerror, qinfo, qtrace, qwarn, Decoder, Header, MessageType, Role}; use neqo_qpack::{decoder::QPackDecoder, encoder::QPackEncoder}; use neqo_transport::{ - streams::SendOrder, AppError, Connection, ConnectionError, DatagramTracking, State, StreamId, + streams::SendOrder, AppError, CloseReason, Connection, DatagramTracking, State, StreamId, StreamType, ZeroRttState, }; @@ -81,22 +81,22 @@ enum Http3RemoteSettingsState { /// - `ZeroRtt`: 0-RTT has been enabled and is active /// - Connected /// - GoingAway(StreamId): The connection has received a `GOAWAY` frame -/// - Closing(ConnectionError): The connection is closed. The closing has been initiated by this end -/// of the connection, e.g., the `CONNECTION_CLOSE` frame has been sent. In this state, the +/// - Closing(CloseReason): The connection is closed. The closing has been initiated by this end of +/// the connection, e.g., the `CONNECTION_CLOSE` frame has been sent. In this state, the /// connection waits a certain amount of time to retransmit the `CONNECTION_CLOSE` frame if /// needed. -/// - Closed(ConnectionError): This is the final close state: closing has been initialized by the -/// peer and an ack for the `CONNECTION_CLOSE` frame has been sent or the closing has been -/// initiated by this end of the connection and the ack for the `CONNECTION_CLOSE` has been -/// received or the waiting time has passed. +/// - Closed(CloseReason): This is the final close state: closing has been initialized by the peer +/// and an ack for the `CONNECTION_CLOSE` frame has been sent or the closing has been initiated by +/// this end of the connection and the ack for the `CONNECTION_CLOSE` has been received or the +/// waiting time has passed. #[derive(Debug, PartialEq, PartialOrd, Ord, Eq, Clone)] pub enum Http3State { Initializing, ZeroRtt, Connected, GoingAway(StreamId), - Closing(ConnectionError), - Closed(ConnectionError), + Closing(CloseReason), + Closed(CloseReason), } impl Http3State { @@ -767,7 +767,7 @@ impl Http3Connection { /// This is called when an application closes the connection. pub fn close(&mut self, error: AppError) { qdebug!([self], "Close connection error {:?}.", error); - self.state = Http3State::Closing(ConnectionError::Application(error)); + self.state = Http3State::Closing(CloseReason::Application(error)); if (!self.send_streams.is_empty() || !self.recv_streams.is_empty()) && (error == 0) { qwarn!("close(0) called when streams still active"); } diff --git a/neqo-http3/src/connection_client.rs b/neqo-http3/src/connection_client.rs index 4c8772d14a..18e513e743 100644 --- a/neqo-http3/src/connection_client.rs +++ b/neqo-http3/src/connection_client.rs @@ -1291,8 +1291,8 @@ mod tests { use neqo_crypto::{AllowZeroRtt, AntiReplay, ResumptionToken}; use neqo_qpack::{encoder::QPackEncoder, QpackSettings}; use neqo_transport::{ - ConnectionError, ConnectionEvent, ConnectionParameters, Output, State, StreamId, - StreamType, Version, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE, + CloseReason, ConnectionEvent, ConnectionParameters, Output, State, StreamId, StreamType, + Version, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE, }; use test_fixture::{ anti_replay, default_server_h3, fixture_init, new_server, now, @@ -1314,7 +1314,7 @@ mod tests { fn assert_closed(client: &Http3Client, expected: &Error) { match client.state() { Http3State::Closing(err) | Http3State::Closed(err) => { - assert_eq!(err, ConnectionError::Application(expected.code())); + assert_eq!(err, CloseReason::Application(expected.code())); } _ => panic!("Wrong state {:?}", client.state()), }; @@ -4419,7 +4419,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 100), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4437,7 +4437,7 @@ mod tests { HSetting::new(HSettingType::MaxTableCapacity, 100), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4474,7 +4474,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 100), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(514)), + &Http3State::Closing(CloseReason::Application(514)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4493,7 +4493,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 100), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4531,7 +4531,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 50), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4569,7 +4569,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 100), HSetting::new(HSettingType::MaxHeaderListSize, 5000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } @@ -4626,7 +4626,7 @@ mod tests { HSetting::new(HSettingType::BlockedStreams, 100), HSetting::new(HSettingType::MaxHeaderListSize, 10000), ], - &Http3State::Closing(ConnectionError::Application(265)), + &Http3State::Closing(CloseReason::Application(265)), ENCODER_STREAM_DATA_WITH_CAP_INSTRUCTION, ); } diff --git a/neqo-http3/src/features/extended_connect/tests/webtransport/negotiation.rs b/neqo-http3/src/features/extended_connect/tests/webtransport/negotiation.rs index 27f669861d..9b54f1dc46 100644 --- a/neqo-http3/src/features/extended_connect/tests/webtransport/negotiation.rs +++ b/neqo-http3/src/features/extended_connect/tests/webtransport/negotiation.rs @@ -8,7 +8,7 @@ use std::time::Duration; use neqo_common::{event::Provider, Encoder}; use neqo_crypto::AuthenticationStatus; -use neqo_transport::{Connection, ConnectionError, StreamType}; +use neqo_transport::{CloseReason, Connection, StreamType}; use test_fixture::{default_server_h3, now}; use super::{connect, default_http3_client, default_http3_server, exchange_packets}; @@ -270,10 +270,7 @@ fn wrong_setting_value() { exchange_packets2(&mut client, &mut server); match client.state() { Http3State::Closing(err) | Http3State::Closed(err) => { - assert_eq!( - err, - ConnectionError::Application(Error::HttpSettings.code()) - ); + assert_eq!(err, CloseReason::Application(Error::HttpSettings.code())); } _ => panic!("Wrong state {:?}", client.state()), }; diff --git a/neqo-http3/src/server.rs b/neqo-http3/src/server.rs index 1396a4e4cf..8fce803fb3 100644 --- a/neqo-http3/src/server.rs +++ b/neqo-http3/src/server.rs @@ -323,7 +323,7 @@ mod tests { use neqo_crypto::{AuthenticationStatus, ZeroRttCheckResult, ZeroRttChecker}; use neqo_qpack::{encoder::QPackEncoder, QpackSettings}; use neqo_transport::{ - Connection, ConnectionError, ConnectionEvent, State, StreamId, StreamType, ZeroRttState, + CloseReason, Connection, ConnectionEvent, State, StreamId, StreamType, ZeroRttState, }; use test_fixture::{ anti_replay, default_client, fixture_init, now, CountingConnectionIdGenerator, @@ -366,7 +366,7 @@ mod tests { } fn assert_closed(hconn: &mut Http3Server, expected: &Error) { - let err = ConnectionError::Application(expected.code()); + let err = CloseReason::Application(expected.code()); let closed = |e| matches!(e, Http3ServerEvent::StateChange{ state: Http3State::Closing(e) | Http3State::Closed(e), .. } if e == err); assert!(hconn.events().any(closed)); } diff --git a/neqo-http3/tests/httpconn.rs b/neqo-http3/tests/httpconn.rs index a0b2bcdb80..c0c62de9c9 100644 --- a/neqo-http3/tests/httpconn.rs +++ b/neqo-http3/tests/httpconn.rs @@ -17,7 +17,7 @@ use neqo_http3::{ Header, Http3Client, Http3ClientEvent, Http3OrWebTransportStream, Http3Parameters, Http3Server, Http3ServerEvent, Http3State, Priority, }; -use neqo_transport::{ConnectionError, ConnectionParameters, Error, Output, StreamType}; +use neqo_transport::{CloseReason, ConnectionParameters, Error, Output, StreamType}; use test_fixture::*; const RESPONSE_DATA: &[u8] = &[0x61, 0x62, 0x63]; @@ -448,7 +448,7 @@ fn fetch_noresponse_will_idletimeout() { if let Http3ClientEvent::StateChange(state) = event { match state { Http3State::Closing(error_code) | Http3State::Closed(error_code) => { - assert_eq!(error_code, ConnectionError::Transport(Error::IdleTimeout)); + assert_eq!(error_code, CloseReason::Transport(Error::IdleTimeout)); done = true; } _ => {} diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 632d7fc866..f955381414 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -58,7 +58,7 @@ use crate::{ }, tracking::{AckTracker, PacketNumberSpace, RecvdPackets, SentPacket}, version::{Version, WireVersion}, - AppError, ConnectionError, Error, Res, StreamId, + AppError, CloseReason, Error, Res, StreamId, }; mod dump; @@ -889,7 +889,7 @@ impl Connection { let msg = format!("{v:?}"); #[cfg(not(debug_assertions))] let msg = ""; - let error = ConnectionError::Transport(v.clone()); + let error = CloseReason::Transport(v.clone()); match &self.state { State::Closing { error: err, .. } | State::Draining { error: err, .. } @@ -960,9 +960,7 @@ impl Connection { let pto = self.pto(); if self.idle_timeout.expired(now, pto) { qinfo!([self], "idle timeout expired"); - self.set_state(State::Closed(ConnectionError::Transport( - Error::IdleTimeout, - ))); + self.set_state(State::Closed(CloseReason::Transport(Error::IdleTimeout))); return; } @@ -1206,7 +1204,7 @@ impl Connection { qdebug!([self], "Stateless reset: {}", hex(&d[d.len() - 16..])); self.state_signaling.reset(); self.set_state(State::Draining { - error: ConnectionError::Transport(Error::StatelessReset), + error: CloseReason::Transport(Error::StatelessReset), timeout: self.get_closing_period_time(now), }); Err(Error::StatelessReset) @@ -1283,7 +1281,7 @@ impl Connection { } else { qinfo!([self], "Version negotiation: failed with {:?}", supported); // This error goes straight to closed. - self.set_state(State::Closed(ConnectionError::Transport( + self.set_state(State::Closed(CloseReason::Transport( Error::VersionNegotiation, ))); Err(Error::VersionNegotiation) @@ -2213,7 +2211,7 @@ impl Connection { ); builder.set_limit(limit); } - // ConnectionError::Application is only allowed at 1RTT. + // CloseReason::Application is only allowed at 1RTT. let sanitized = if space == PacketNumberSpace::ApplicationData { None } else { @@ -2429,7 +2427,7 @@ impl Connection { /// Close the connection. pub fn close(&mut self, now: Instant, app_error: AppError, msg: impl AsRef) { - let error = ConnectionError::Application(app_error); + let error = CloseReason::Application(app_error); let timeout = self.get_closing_period_time(now); if let Some(path) = self.paths.primary() { self.state_signaling.close(path, error.clone(), 0, msg); @@ -2848,7 +2846,7 @@ impl Connection { FRAME_TYPE_CONNECTION_CLOSE_TRANSPORT, ) }; - let error = ConnectionError::Transport(detail); + let error = CloseReason::Transport(detail); self.state_signaling .drain(Rc::clone(path), error.clone(), frame_type, ""); self.set_state(State::Draining { diff --git a/neqo-transport/src/connection/state.rs b/neqo-transport/src/connection/state.rs index ecf91abd07..9f8f2d4f5c 100644 --- a/neqo-transport/src/connection/state.rs +++ b/neqo-transport/src/connection/state.rs @@ -21,7 +21,7 @@ use crate::{ packet::PacketBuilder, path::PathRef, recovery::RecoveryToken, - ConnectionError, Error, + CloseReason, Error, }; #[derive(Clone, Debug, PartialEq, Eq)] @@ -42,14 +42,14 @@ pub enum State { Connected, Confirmed, Closing { - error: ConnectionError, + error: CloseReason, timeout: Instant, }, Draining { - error: ConnectionError, + error: CloseReason, timeout: Instant, }, - Closed(ConnectionError), + Closed(CloseReason), } impl State { @@ -67,7 +67,7 @@ impl State { } #[must_use] - pub fn error(&self) -> Option<&ConnectionError> { + pub fn error(&self) -> Option<&CloseReason> { if let Self::Closing { error, .. } | Self::Draining { error, .. } | Self::Closed(error) = self { @@ -116,7 +116,7 @@ impl Ord for State { #[derive(Debug, Clone)] pub struct ClosingFrame { path: PathRef, - error: ConnectionError, + error: CloseReason, frame_type: FrameType, reason_phrase: Vec, } @@ -124,7 +124,7 @@ pub struct ClosingFrame { impl ClosingFrame { fn new( path: PathRef, - error: ConnectionError, + error: CloseReason, frame_type: FrameType, message: impl AsRef, ) -> Self { @@ -142,12 +142,12 @@ impl ClosingFrame { } pub fn sanitize(&self) -> Option { - if let ConnectionError::Application(_) = self.error { + if let CloseReason::Application(_) = self.error { // The default CONNECTION_CLOSE frame that is sent when an application // error code needs to be sent in an Initial or Handshake packet. Some(Self { path: Rc::clone(&self.path), - error: ConnectionError::Transport(Error::ApplicationError), + error: CloseReason::Transport(Error::ApplicationError), frame_type: 0, reason_phrase: Vec::new(), }) @@ -166,12 +166,12 @@ impl ClosingFrame { return; } match &self.error { - ConnectionError::Transport(e) => { + CloseReason::Transport(e) => { builder.encode_varint(FRAME_TYPE_CONNECTION_CLOSE_TRANSPORT); builder.encode_varint(e.code()); builder.encode_varint(self.frame_type); } - ConnectionError::Application(code) => { + CloseReason::Application(code) => { builder.encode_varint(FRAME_TYPE_CONNECTION_CLOSE_APPLICATION); builder.encode_varint(*code); } @@ -234,7 +234,7 @@ impl StateSignaling { pub fn close( &mut self, path: PathRef, - error: ConnectionError, + error: CloseReason, frame_type: FrameType, message: impl AsRef, ) { @@ -246,7 +246,7 @@ impl StateSignaling { pub fn drain( &mut self, path: PathRef, - error: ConnectionError, + error: CloseReason, frame_type: FrameType, message: impl AsRef, ) { diff --git a/neqo-transport/src/connection/tests/close.rs b/neqo-transport/src/connection/tests/close.rs index ba6e5548d1..7c620de17e 100644 --- a/neqo-transport/src/connection/tests/close.rs +++ b/neqo-transport/src/connection/tests/close.rs @@ -14,13 +14,13 @@ use super::{ }; use crate::{ tparams::{self, TransportParameter}, - AppError, ConnectionError, Error, ERROR_APPLICATION_CLOSE, + AppError, CloseReason, Error, ERROR_APPLICATION_CLOSE, }; fn assert_draining(c: &Connection, expected: &Error) { assert!(c.state().closed()); if let State::Draining { - error: ConnectionError::Transport(error), + error: CloseReason::Transport(error), .. } = c.state() { @@ -114,7 +114,7 @@ fn bad_tls_version() { let dgram = server.process(dgram.as_ref(), now()).dgram(); assert_eq!( *server.state(), - State::Closed(ConnectionError::Transport(Error::ProtocolViolation)) + State::Closed(CloseReason::Transport(Error::ProtocolViolation)) ); assert!(dgram.is_some()); client.process_input(&dgram.unwrap(), now()); @@ -168,7 +168,6 @@ fn closing_and_draining() { assert!(client_close.is_some()); let client_close_timer = client.process(None, now()).callback(); assert_ne!(client_close_timer, Duration::from_secs(0)); - // The client will spit out the same packet in response to anything it receives. let p3 = send_something(&mut server, now()); let client_close2 = client.process(Some(&p3), now()).dgram(); @@ -182,7 +181,7 @@ fn closing_and_draining() { assert_eq!(end, Output::None); assert_eq!( *client.state(), - State::Closed(ConnectionError::Application(APP_ERROR)) + State::Closed(CloseReason::Application(APP_ERROR)) ); // When the server receives the close, it too should generate CONNECTION_CLOSE. @@ -200,7 +199,7 @@ fn closing_and_draining() { assert_eq!(end, Output::None); assert_eq!( *server.state(), - State::Closed(ConnectionError::Transport(Error::PeerApplicationError( + State::Closed(CloseReason::Transport(Error::PeerApplicationError( APP_ERROR ))) ); diff --git a/neqo-transport/src/connection/tests/datagram.rs b/neqo-transport/src/connection/tests/datagram.rs index f80c7d9104..f1b64b3c8f 100644 --- a/neqo-transport/src/connection/tests/datagram.rs +++ b/neqo-transport/src/connection/tests/datagram.rs @@ -19,7 +19,7 @@ use crate::{ packet::PacketBuilder, quic_datagrams::MAX_QUIC_DATAGRAM, send_stream::{RetransmissionPriority, TransmissionPriority}, - Connection, ConnectionError, ConnectionParameters, Error, StreamType, + CloseReason, Connection, ConnectionParameters, Error, StreamType, }; const DATAGRAM_LEN_MTU: u64 = 1310; @@ -362,10 +362,7 @@ fn dgram_no_allowed() { client.process_input(&out, now()); - assert_error( - &client, - &ConnectionError::Transport(Error::ProtocolViolation), - ); + assert_error(&client, &CloseReason::Transport(Error::ProtocolViolation)); } #[test] @@ -383,10 +380,7 @@ fn dgram_too_big() { client.process_input(&out, now()); - assert_error( - &client, - &ConnectionError::Transport(Error::ProtocolViolation), - ); + assert_error(&client, &CloseReason::Transport(Error::ProtocolViolation)); } #[test] diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index f2103523ec..c908340616 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/neqo-transport/src/connection/tests/handshake.rs @@ -35,7 +35,7 @@ use crate::{ server::ValidateAddress, tparams::{TransportParameter, MIN_ACK_DELAY}, tracking::DEFAULT_ACK_DELAY, - ConnectionError, ConnectionParameters, EmptyConnectionIdGenerator, Error, StreamType, Version, + CloseReason, ConnectionParameters, EmptyConnectionIdGenerator, Error, StreamType, Version, }; const ECH_CONFIG_ID: u8 = 7; @@ -111,8 +111,8 @@ fn handshake_failed_authentication() { qdebug!("---- server: Alert(certificate_revoked)"); let out = server.process(out.as_dgram_ref(), now()); assert!(out.as_dgram_ref().is_some()); - assert_error(&client, &ConnectionError::Transport(Error::CryptoAlert(44))); - assert_error(&server, &ConnectionError::Transport(Error::PeerError(300))); + assert_error(&client, &CloseReason::Transport(Error::CryptoAlert(44))); + assert_error(&server, &CloseReason::Transport(Error::PeerError(300))); } #[test] @@ -133,11 +133,8 @@ fn no_alpn() { handshake(&mut client, &mut server, now(), Duration::new(0, 0)); // TODO (mt): errors are immediate, which means that we never send CONNECTION_CLOSE // and the client never sees the server's rejection of its handshake. - // assert_error(&client, ConnectionError::Transport(Error::CryptoAlert(120))); - assert_error( - &server, - &ConnectionError::Transport(Error::CryptoAlert(120)), - ); + // assert_error(&client, CloseReason::Transport(Error::CryptoAlert(120))); + assert_error(&server, &CloseReason::Transport(Error::CryptoAlert(120))); } #[test] @@ -934,10 +931,10 @@ fn ech_retry() { server.process_input(&dgram.unwrap(), now()); assert_eq!( server.state().error(), - Some(&ConnectionError::Transport(Error::PeerError(0x100 + 121))) + Some(&CloseReason::Transport(Error::PeerError(0x100 + 121))) ); - let Some(ConnectionError::Transport(Error::EchRetry(updated_config))) = client.state().error() + let Some(CloseReason::Transport(Error::EchRetry(updated_config))) = client.state().error() else { panic!( "Client state should be failed with EchRetry, is {:?}", @@ -984,7 +981,7 @@ fn ech_retry_fallback_rejected() { client.authenticated(AuthenticationStatus::PolicyRejection, now()); assert!(client.state().error().is_some()); - if let Some(ConnectionError::Transport(Error::EchRetry(_))) = client.state().error() { + if let Some(CloseReason::Transport(Error::EchRetry(_))) = client.state().error() { panic!("Client should not get EchRetry error"); } @@ -993,14 +990,13 @@ fn ech_retry_fallback_rejected() { server.process_input(&dgram.unwrap(), now()); assert_eq!( server.state().error(), - Some(&ConnectionError::Transport(Error::PeerError(298))) + Some(&CloseReason::Transport(Error::PeerError(298))) ); // A bad_certificate alert. } #[test] fn bad_min_ack_delay() { - const EXPECTED_ERROR: ConnectionError = - ConnectionError::Transport(Error::TransportParameterError); + const EXPECTED_ERROR: CloseReason = CloseReason::Transport(Error::TransportParameterError); let mut server = default_server(); let max_ad = u64::try_from(DEFAULT_ACK_DELAY.as_micros()).unwrap(); server @@ -1018,7 +1014,7 @@ fn bad_min_ack_delay() { server.process_input(&dgram.unwrap(), now()); assert_eq!( server.state().error(), - Some(&ConnectionError::Transport(Error::PeerError( + Some(&CloseReason::Transport(Error::PeerError( Error::TransportParameterError.code() ))) ); diff --git a/neqo-transport/src/connection/tests/keys.rs b/neqo-transport/src/connection/tests/keys.rs index 847b253284..c2ae9529bf 100644 --- a/neqo-transport/src/connection/tests/keys.rs +++ b/neqo-transport/src/connection/tests/keys.rs @@ -11,7 +11,7 @@ use test_fixture::now; use super::{ super::{ - super::{ConnectionError, ERROR_AEAD_LIMIT_REACHED}, + super::{CloseReason, ERROR_AEAD_LIMIT_REACHED}, Connection, ConnectionParameters, Error, Output, State, StreamType, }, connect, connect_force_idle, default_client, default_server, maybe_authenticate, @@ -269,7 +269,7 @@ fn exhaust_write_keys() { assert!(dgram.is_none()); assert!(matches!( client.state(), - State::Closed(ConnectionError::Transport(Error::KeysExhausted)) + State::Closed(CloseReason::Transport(Error::KeysExhausted)) )); } @@ -285,14 +285,14 @@ fn exhaust_read_keys() { let dgram = server.process(Some(&dgram), now()).dgram(); assert!(matches!( server.state(), - State::Closed(ConnectionError::Transport(Error::KeysExhausted)) + State::Closed(CloseReason::Transport(Error::KeysExhausted)) )); client.process_input(&dgram.unwrap(), now()); assert!(matches!( client.state(), State::Draining { - error: ConnectionError::Transport(Error::PeerError(ERROR_AEAD_LIMIT_REACHED)), + error: CloseReason::Transport(Error::PeerError(ERROR_AEAD_LIMIT_REACHED)), .. } )); @@ -341,6 +341,6 @@ fn automatic_update_write_keys_blocked() { assert!(dgram.is_none()); assert!(matches!( client.state(), - State::Closed(ConnectionError::Transport(Error::KeysExhausted)) + State::Closed(CloseReason::Transport(Error::KeysExhausted)) )); } diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 5f7136ca9f..779cc78c53 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/neqo-transport/src/connection/tests/migration.rs @@ -30,7 +30,7 @@ use crate::{ packet::PacketBuilder, path::{PATH_MTU_V4, PATH_MTU_V6}, tparams::{self, PreferredAddress, TransportParameter}, - ConnectionError, ConnectionId, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionIdRef, + CloseReason, ConnectionId, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionIdRef, ConnectionParameters, EmptyConnectionIdGenerator, Error, }; @@ -357,7 +357,7 @@ fn migrate_same_fail() { assert!(matches!(res, Output::None)); assert!(matches!( client.state(), - State::Closed(ConnectionError::Transport(Error::NoAvailablePath)) + State::Closed(CloseReason::Transport(Error::NoAvailablePath)) )); } @@ -894,7 +894,7 @@ fn retire_prior_to_migration_failure() { assert!(matches!( client.state(), State::Closing { - error: ConnectionError::Transport(Error::InvalidMigration), + error: CloseReason::Transport(Error::InvalidMigration), .. } )); diff --git a/neqo-transport/src/connection/tests/mod.rs b/neqo-transport/src/connection/tests/mod.rs index 59c3898660..65283b8eb8 100644 --- a/neqo-transport/src/connection/tests/mod.rs +++ b/neqo-transport/src/connection/tests/mod.rs @@ -17,7 +17,7 @@ use neqo_common::{event::Provider, qdebug, qtrace, Datagram, Decoder, Role}; use neqo_crypto::{random, AllowZeroRtt, AuthenticationStatus, ResumptionToken}; use test_fixture::{fixture_init, new_neqo_qlog, now, DEFAULT_ADDR}; -use super::{Connection, ConnectionError, ConnectionId, Output, State}; +use super::{CloseReason, Connection, ConnectionId, Output, State}; use crate::{ addr_valid::{AddressValidation, ValidateAddress}, cc::{CWND_INITIAL_PKTS, CWND_MIN}, @@ -245,8 +245,8 @@ fn connect_fail( server_error: Error, ) { handshake(client, server, now(), Duration::new(0, 0)); - assert_error(client, &ConnectionError::Transport(client_error)); - assert_error(server, &ConnectionError::Transport(server_error)); + assert_error(client, &CloseReason::Transport(client_error)); + assert_error(server, &CloseReason::Transport(server_error)); } fn connect_with_rtt_and_modifier( @@ -284,7 +284,7 @@ fn connect(client: &mut Connection, server: &mut Connection) { connect_with_rtt(client, server, now(), Duration::new(0, 0)); } -fn assert_error(c: &Connection, expected: &ConnectionError) { +fn assert_error(c: &Connection, expected: &CloseReason) { match c.state() { State::Closing { error, .. } | State::Draining { error, .. } | State::Closed(error) => { assert_eq!(*error, *expected, "{c} error mismatch"); diff --git a/neqo-transport/src/connection/tests/stream.rs b/neqo-transport/src/connection/tests/stream.rs index 66d3bf32f3..f7472d917f 100644 --- a/neqo-transport/src/connection/tests/stream.rs +++ b/neqo-transport/src/connection/tests/stream.rs @@ -19,9 +19,9 @@ use crate::{ send_stream::{OrderGroup, SendStreamState, SEND_BUFFER_SIZE}, streams::{SendOrder, StreamOrder}, tparams::{self, TransportParameter}, + CloseReason, // tracking::DEFAULT_ACK_PACKET_TOLERANCE, Connection, - ConnectionError, ConnectionParameters, Error, StreamId, @@ -494,12 +494,9 @@ fn exceed_max_data() { assert_error( &client, - &ConnectionError::Transport(Error::PeerError(Error::FlowControlError.code())), - ); - assert_error( - &server, - &ConnectionError::Transport(Error::FlowControlError), + &CloseReason::Transport(Error::PeerError(Error::FlowControlError.code())), ); + assert_error(&server, &CloseReason::Transport(Error::FlowControlError)); } #[test] diff --git a/neqo-transport/src/connection/tests/vn.rs b/neqo-transport/src/connection/tests/vn.rs index 93872a94f4..815868d78d 100644 --- a/neqo-transport/src/connection/tests/vn.rs +++ b/neqo-transport/src/connection/tests/vn.rs @@ -10,7 +10,7 @@ use neqo_common::{event::Provider, Decoder, Encoder}; use test_fixture::{assertions, datagram, now}; use super::{ - super::{ConnectionError, ConnectionEvent, Output, State, ZeroRttState}, + super::{CloseReason, ConnectionEvent, Output, State, ZeroRttState}, connect, connect_fail, default_client, default_server, exchange_ticket, new_client, new_server, send_something, }; @@ -124,7 +124,7 @@ fn version_negotiation_only_reserved() { assert_eq!(client.process(Some(&dgram), now()), Output::None); match client.state() { State::Closed(err) => { - assert_eq!(*err, ConnectionError::Transport(Error::VersionNegotiation)); + assert_eq!(*err, CloseReason::Transport(Error::VersionNegotiation)); } _ => panic!("Invalid client state"), } @@ -183,7 +183,7 @@ fn version_negotiation_not_supported() { assert_eq!(client.process(Some(&dgram), now()), Output::None); match client.state() { State::Closed(err) => { - assert_eq!(*err, ConnectionError::Transport(Error::VersionNegotiation)); + assert_eq!(*err, CloseReason::Transport(Error::VersionNegotiation)); } _ => panic!("Invalid client state"), } @@ -338,7 +338,7 @@ fn invalid_server_version() { // The server effectively hasn't reacted here. match server.state() { State::Closed(err) => { - assert_eq!(*err, ConnectionError::Transport(Error::CryptoAlert(47))); + assert_eq!(*err, CloseReason::Transport(Error::CryptoAlert(47))); } _ => panic!("invalid server state"), } diff --git a/neqo-transport/src/events.rs b/neqo-transport/src/events.rs index a892e384b9..68ef0d6798 100644 --- a/neqo-transport/src/events.rs +++ b/neqo-transport/src/events.rs @@ -256,7 +256,7 @@ impl EventProvider for ConnectionEvents { mod tests { use neqo_common::event::Provider; - use crate::{ConnectionError, ConnectionEvent, ConnectionEvents, Error, State, StreamId}; + use crate::{CloseReason, ConnectionEvent, ConnectionEvents, Error, State, StreamId}; #[test] fn event_culling() { @@ -314,7 +314,7 @@ mod tests { evts.send_stream_writable(9.into()); evts.send_stream_stop_sending(10.into(), 55); - evts.connection_state_change(State::Closed(ConnectionError::Transport( + evts.connection_state_change(State::Closed(CloseReason::Transport( Error::StreamStateError, ))); assert_eq!(evts.events().count(), 1); diff --git a/neqo-transport/src/frame.rs b/neqo-transport/src/frame.rs index 279bfa5c25..7d009f3b46 100644 --- a/neqo-transport/src/frame.rs +++ b/neqo-transport/src/frame.rs @@ -15,7 +15,7 @@ use crate::{ ecn::EcnCount, packet::PacketType, stream_id::{StreamId, StreamType}, - AppError, ConnectionError, Error, Res, TransportError, + AppError, CloseReason, Error, Res, TransportError, }; #[allow(clippy::module_name_repetitions)] @@ -87,11 +87,11 @@ impl CloseError { } } -impl From for CloseError { - fn from(err: ConnectionError) -> Self { +impl From for CloseError { + fn from(err: CloseReason) -> Self { match err { - ConnectionError::Transport(c) => Self::Transport(c.code()), - ConnectionError::Application(c) => Self::Application(c), + CloseReason::Transport(c) => Self::Transport(c.code()), + CloseReason::Application(c) => Self::Application(c), } } } diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index f0d126569a..723a86980e 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -209,13 +209,17 @@ impl ::std::fmt::Display for Error { pub type AppError = u64; +#[deprecated(note = "use `CloseReason` instead")] +pub type ConnectionError = CloseReason; + +/// Reason why a connection closed. #[derive(Clone, Debug, PartialEq, PartialOrd, Ord, Eq)] -pub enum ConnectionError { +pub enum CloseReason { Transport(Error), Application(AppError), } -impl ConnectionError { +impl CloseReason { #[must_use] pub fn app_code(&self) -> Option { match self { @@ -225,17 +229,17 @@ impl ConnectionError { } /// Checks enclosed error for [`Error::NoError`] and - /// [`ConnectionError::Application(0)`]. + /// [`CloseReason::Application(0)`]. #[must_use] pub fn is_error(&self) -> bool { !matches!( self, - ConnectionError::Transport(Error::NoError) | ConnectionError::Application(0), + CloseReason::Transport(Error::NoError) | CloseReason::Application(0), ) } } -impl From for ConnectionError { +impl From for CloseReason { fn from(err: CloseError) -> Self { match err { CloseError::Transport(c) => Self::Transport(Error::PeerError(c)), diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index d08d946cf8..3cc711f80b 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -7,7 +7,7 @@ mod common; use neqo_common::{Datagram, Decoder, Encoder, Role}; -use neqo_transport::{ConnectionError, ConnectionParameters, Error, State, Version}; +use neqo_transport::{CloseReason, ConnectionParameters, Error, State, Version}; use test_fixture::{ default_client, default_server, header_protection::{ @@ -180,7 +180,7 @@ fn packet_without_frames() { client.process_input(&modified, now()); assert_eq!( client.state(), - &State::Closed(ConnectionError::Transport(Error::ProtocolViolation)) + &State::Closed(CloseReason::Transport(Error::ProtocolViolation)) ); } @@ -266,10 +266,7 @@ fn overflow_crypto() { client.process_input(&dgram, now()); if let State::Closing { error, .. } = client.state() { assert!( - matches!( - error, - ConnectionError::Transport(Error::CryptoBufferExceeded), - ), + matches!(error, CloseReason::Transport(Error::CryptoBufferExceeded),), "the connection need to abort on crypto buffer" ); assert!(pn > 64, "at least 64000 bytes of data is buffered"); diff --git a/neqo-transport/tests/network.rs b/neqo-transport/tests/network.rs index 27e5a83cd6..68a835a436 100644 --- a/neqo-transport/tests/network.rs +++ b/neqo-transport/tests/network.rs @@ -6,7 +6,7 @@ use std::{ops::Range, time::Duration}; -use neqo_transport::{ConnectionError, ConnectionParameters, Error, State}; +use neqo_transport::{CloseReason, ConnectionParameters, Error, State}; use test_fixture::{ boxed, sim::{ @@ -48,10 +48,10 @@ simulate!( idle_timeout, [ ConnectionNode::default_client(boxed![ReachState::new(State::Closed( - ConnectionError::Transport(Error::IdleTimeout) + CloseReason::Transport(Error::IdleTimeout) ))]), ConnectionNode::default_server(boxed![ReachState::new(State::Closed( - ConnectionError::Transport(Error::IdleTimeout) + CloseReason::Transport(Error::IdleTimeout) ))]), ] ); @@ -62,7 +62,7 @@ simulate!( ConnectionNode::new_client( ConnectionParameters::default().idle_timeout(weeks(1000)), boxed![ReachState::new(State::Confirmed),], - boxed![ReachState::new(State::Closed(ConnectionError::Transport( + boxed![ReachState::new(State::Closed(CloseReason::Transport( Error::IdleTimeout )))] ), @@ -71,7 +71,7 @@ simulate!( ConnectionNode::new_server( ConnectionParameters::default().idle_timeout(weeks(1000)), boxed![ReachState::new(State::Confirmed),], - boxed![ReachState::new(State::Closed(ConnectionError::Transport( + boxed![ReachState::new(State::Closed(CloseReason::Transport( Error::IdleTimeout )))] ), diff --git a/neqo-transport/tests/retry.rs b/neqo-transport/tests/retry.rs index 0cfc48f051..3f95511c3e 100644 --- a/neqo-transport/tests/retry.rs +++ b/neqo-transport/tests/retry.rs @@ -17,7 +17,7 @@ use std::{ use common::{connected_server, default_server, generate_ticket}; use neqo_common::{hex_with_len, qdebug, qtrace, Datagram, Encoder, Role}; use neqo_crypto::AuthenticationStatus; -use neqo_transport::{server::ValidateAddress, ConnectionError, Error, State, StreamType}; +use neqo_transport::{server::ValidateAddress, CloseReason, Error, State, StreamType}; use test_fixture::{ assertions, datagram, default_client, header_protection::{ @@ -469,7 +469,7 @@ fn mitm_retry() { assert!(matches!( *client.state(), State::Closing { - error: ConnectionError::Transport(Error::ProtocolViolation), + error: CloseReason::Transport(Error::ProtocolViolation), .. } )); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 3c43a9105d..4740d26ded 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -15,7 +15,7 @@ use neqo_crypto::{ }; use neqo_transport::{ server::{ActiveConnectionRef, Server, ValidateAddress}, - Connection, ConnectionError, ConnectionParameters, Error, Output, State, StreamType, Version, + CloseReason, Connection, ConnectionParameters, Error, Output, State, StreamType, Version, }; use test_fixture::{ assertions, datagram, default_client, @@ -463,13 +463,13 @@ fn bad_client_initial() { assert_ne!(delay, Duration::from_secs(0)); assert!(matches!( *client.state(), - State::Draining { error: ConnectionError::Transport(Error::PeerError(code)), .. } if code == Error::ProtocolViolation.code() + State::Draining { error: CloseReason::Transport(Error::PeerError(code)), .. } if code == Error::ProtocolViolation.code() )); for server in server.active_connections() { assert_eq!( *server.borrow().state(), - State::Closed(ConnectionError::Transport(Error::ProtocolViolation)) + State::Closed(CloseReason::Transport(Error::ProtocolViolation)) ); } From 47588ac0dff57344943f6fc2d9c4bf7f3586a01d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 6 May 2024 07:24:02 +0200 Subject: [PATCH 10/13] refactor: remove unused test-fixture/src/sim/net.rs (#1873) The `net.rs` file is not declared as a module in `src/sim/mod.rs` and thus unused. --- test-fixture/src/sim/net.rs | 111 ------------------------------------ 1 file changed, 111 deletions(-) delete mode 100644 test-fixture/src/sim/net.rs diff --git a/test-fixture/src/sim/net.rs b/test-fixture/src/sim/net.rs deleted file mode 100644 index 754426f895..0000000000 --- a/test-fixture/src/sim/net.rs +++ /dev/null @@ -1,111 +0,0 @@ -// Licensed under the Apache License, Version 2.0 or the MIT license -// , at your -// option. This file may not be copied, modified, or distributed -// except according to those terms. - -use super::rng::RandomDuration; -use super::{Node, Rng}; -use neqo_common::Datagram; -use neqo_transport::Output; -use std::collections::BTreeMap; -use std::fmt::{self, Debug}; -use std::iter; -use std::ops::Range; -use std::time::{Duration, Instant}; - -/// -pub struct RandomDrop { - threshold: u64, - max: u64, - rng: Rng, -} - -impl RandomDuration { - /// Make a new random `Duration` generator. This asserts if the range provided - /// is inverted (i.e., `bounds.start > bounds.end`), or spans 2^64 - /// or more nanoseconds. - /// A zero-length range means that random values won't be taken from the Rng - pub fn new(bounds: Range, rng: Rng) -> Self { - let max = u64::try_from((bounds.end - bounds.start).as_nanos()).unwrap(); - Self { - start: bounds.start, - max, - rng, - } - } - - fn next(&mut self) -> Duration { - let r = if self.max == 0 { - Duration::new(0, 0) - } else { - self.rng.borrow_mut().random_from(0..self.max) - } - self.start + Duration::from_nanos(r) - } -} - -enum DelayState { - New(Range), - Ready(RandomDuration), -} - -pub struct Delay { - state: DelayState, - queue: BTreeMap, -} - -impl Delay -{ - pub fn new(bounds: Range) -> Self - { - Self { - State: DelayState::New(bounds), - queue: BTreeMap::default(), - } - } - - fn insert(&mut self, d: Datagram, now: Instant) { - let mut t = if let State::Ready(r) = self.state { - now + self.source.next() - } else { - unreachable!(); - } - while self.queue.contains_key(&t) { - // This is a little inefficient, but it avoids drops on collisions, - // which are super-common for a fixed delay. - t += Duration::from_nanos(1); - } - self.queue.insert(t, d); - } -} - -impl Node for Delay -{ - fn init(&mut self, rng: Rng, now: Instant) { - if let DelayState::New(bounds) = self.state { - self.state = RandomDuration::new(bounds); - } else { - unreachable!(); - } - } - - fn process(&mut self, d: Option, now: Instant) -> Output { - if let Some(dgram) = d { - self.insert(dgram, now); - } - if let Some((&k, _)) = self.queue.range(..now).nth(0) { - Output::Datagram(self.queue.remove(&k).unwrap()) - } else if let Some(&t) = self.queue.keys().nth(0) { - Output::Callback(t - now) - } else { - Output::None - } - } -} - -impl Debug for Delay { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.write_str("delay") - } -} From e467273e1a08a355c3681a7470a7233853e04cac Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 6 May 2024 07:25:53 +0200 Subject: [PATCH 11/13] fix(http3): always qlog on send_buffer() (#1876) * fix(http3): always qlog on send_buffer() `neqo_http3::SendMessage` calls `qlog::h3_data_moved_down()` whenever it moves data down to the QUIC layer. `SendMessage` moves data down to the QUIC layer either directly via `self.stream.send_atomic` or indirectly buffered through `self.stream.send_buffer`. Previously only one of the 3 calls to `self.stream.send_buffer` would thereafter call `qlog::h3_data_moved_down()`. This commit moves the `h3_data_moved_down` call into `self.stream.send_buffer`, thus ensuring the function is always called when data is moved. In addition, `self.stream.send_atomic` now as well does the qlog call, thus containing all qlog logic in `buffered_send_stream.rs` instead of `send_message.rs`. * Trigger benchmark run * Don't qlog if buffer is empty * Fix typo * Use early return to keep indentation short * Don't qlog if sent is 0 --- neqo-http3/src/buffered_send_stream.rs | 51 ++++++++++++++------------ neqo-http3/src/send_message.rs | 4 +- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index 4f6761fa80..60da0512b5 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -7,7 +7,7 @@ use neqo_common::qtrace; use neqo_transport::{Connection, StreamId}; -use crate::Res; +use crate::{qlog, Res}; #[derive(Debug, PartialEq, Eq)] pub enum BufferedStream { @@ -38,7 +38,7 @@ impl BufferedStream { /// # Panics /// - /// If the `BufferedStream` is initialized more than one it will panic. + /// If the `BufferedStream` is initialized more than once, it will panic. pub fn init(&mut self, stream_id: StreamId) { debug_assert!(&Self::Uninitialized == self); *self = Self::Initialized { @@ -63,19 +63,23 @@ impl BufferedStream { /// Returns `neqo_transport` errors. pub fn send_buffer(&mut self, conn: &mut Connection) -> Res { let label = ::neqo_common::log_subject!(::log::Level::Debug, self); - let mut sent = 0; - if let Self::Initialized { stream_id, buf } = self { - if !buf.is_empty() { - qtrace!([label], "sending data."); - sent = conn.stream_send(*stream_id, &buf[..])?; - if sent == buf.len() { - buf.clear(); - } else { - let b = buf.split_off(sent); - *buf = b; - } - } + let Self::Initialized { stream_id, buf } = self else { + return Ok(0); + }; + if buf.is_empty() { + return Ok(0); + } + qtrace!([label], "sending data."); + let sent = conn.stream_send(*stream_id, &buf[..])?; + if sent == 0 { + return Ok(0); + } else if sent == buf.len() { + buf.clear(); + } else { + let b = buf.split_off(sent); + *buf = b; } + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); Ok(sent) } @@ -85,16 +89,17 @@ impl BufferedStream { pub fn send_atomic(&mut self, conn: &mut Connection, to_send: &[u8]) -> Res { // First try to send anything that is in the buffer. self.send_buffer(conn)?; - if let Self::Initialized { stream_id, buf } = self { - if buf.is_empty() { - let res = conn.stream_send_atomic(*stream_id, to_send)?; - Ok(res) - } else { - Ok(false) - } - } else { - Ok(false) + let Self::Initialized { stream_id, buf } = self else { + return Ok(false); + }; + if !buf.is_empty() { + return Ok(false); + } + let res = conn.stream_send_atomic(*stream_id, to_send)?; + if res { + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, to_send.len()); } + Ok(res) } #[must_use] diff --git a/neqo-http3/src/send_message.rs b/neqo-http3/src/send_message.rs index 15965c44f6..7fb37beb70 100644 --- a/neqo-http3/src/send_message.rs +++ b/neqo-http3/src/send_message.rs @@ -13,7 +13,7 @@ use neqo_transport::{Connection, StreamId}; use crate::{ frames::HFrame, headers_checks::{headers_valid, is_interim, trailers_valid}, - qlog, BufferedStream, CloseType, Error, Http3StreamInfo, Http3StreamType, HttpSendStream, Res, + BufferedStream, CloseType, Error, Http3StreamInfo, Http3StreamType, HttpSendStream, Res, SendStream, SendStreamEvents, Stream, }; @@ -216,7 +216,6 @@ impl SendStream for SendMessage { .send_atomic(conn, &buf[..to_send]) .map_err(|e| Error::map_stream_send_errors(&e))?; debug_assert!(sent); - qlog::h3_data_moved_down(conn.qlog_mut(), self.stream_id(), to_send); Ok(to_send) } @@ -243,7 +242,6 @@ impl SendStream for SendMessage { /// info that the stream has been closed.) fn send(&mut self, conn: &mut Connection) -> Res<()> { let sent = Error::map_error(self.stream.send_buffer(conn), Error::HttpInternal(5))?; - qlog::h3_data_moved_down(conn.qlog_mut(), self.stream_id(), sent); qtrace!([self], "{} bytes sent", sent); if !self.stream.has_buffered_data() { From c42597d07cae31d5b5d5680fb04cd1dfdc1a78da Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 6 May 2024 08:44:19 +0000 Subject: [PATCH 12/13] build(deps): bump codecov/codecov-action from 4.3.0 to 4.3.1 (#1881) Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4.3.0 to 4.3.1. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/codecov/codecov-action/compare/84508663e988701840491b86de86b666e8a86bed...5ecb98a3c6b747ed38dc09f787459979aebb39be) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/check.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 99ba122d2d..4da09d9f4f 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -152,7 +152,7 @@ jobs: if: success() || failure() - name: Upload coverage reports to Codecov - uses: codecov/codecov-action@84508663e988701840491b86de86b666e8a86bed # v4.3.0 + uses: codecov/codecov-action@5ecb98a3c6b747ed38dc09f787459979aebb39be # v4.3.1 with: file: lcov.info fail_ci_if_error: false From adb650785b4693275303be5ecd53a344b768fdeb Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 6 May 2024 08:54:38 +0000 Subject: [PATCH 13/13] build(deps): bump actions/download-artifact from 4.1.6 to 4.1.7 (#1879) * build(deps): bump actions/download-artifact from 4.1.6 to 4.1.7 Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.6 to 4.1.7. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](https://github.com/actions/download-artifact/compare/9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395...65a9edc5881444af0b9093a5e628f2fe47ea3b2e) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] * Addition --------- Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lars Eggert --- .github/actions/pr-comment/action.yml | 2 +- .github/workflows/qns.yml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/actions/pr-comment/action.yml b/.github/actions/pr-comment/action.yml index 6ee1e7d813..fce0bd3dd7 100644 --- a/.github/actions/pr-comment/action.yml +++ b/.github/actions/pr-comment/action.yml @@ -15,7 +15,7 @@ inputs: runs: using: composite steps: - - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 + - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 with: run-id: ${{ github.event.workflow_run.id }} name: ${{ inputs.name }} diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 904eab03c3..275f53bb17 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -129,7 +129,7 @@ jobs: pair: ${{ fromJson(needs.implementations.outputs.pairs) }} runs-on: ubuntu-latest steps: - - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 + - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 with: name: '${{ env.LATEST }} Docker image' path: /tmp @@ -164,7 +164,7 @@ jobs: restore-keys: qns-${{ runner.os }}- - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 + - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 with: pattern: '*results' path: results