From 0a84268b6ebae3a709bba1b9a5d6ab53f5d3501a Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 24 Apr 2024 13:09:08 +0300 Subject: [PATCH 01/16] ci: Pin deps and limit token permissions (#1852) * ci: Pin deps Fixes #1843 * permissions: read-all * Fix * Again * Again * pin rustup * Cannot pin ilammy/msvc-dev-cmd * Again * rustup * Again * Again * Fix merge * Again --------- Signed-off-by: Lars Eggert --- .github/actions/nss/action.yml | 4 ++-- .../actions/pr-comment-data-export/action.yml | 2 +- .github/actions/quic-interop-runner/action.yml | 4 ++-- .github/workflows/actionlint.yml | 4 +++- .github/workflows/bench-comment.yml | 4 +++- .github/workflows/bench.yml | 12 +++++++----- .github/workflows/check.yml | 8 ++++++-- .github/workflows/mutants.yml | 6 ++++-- .github/workflows/qns-comment.yml | 4 +++- .github/workflows/qns.yml | 18 ++++++++++-------- qns/Dockerfile | 15 +++++++++++---- 11 files changed, 52 insertions(+), 29 deletions(-) diff --git a/.github/actions/nss/action.yml b/.github/actions/nss/action.yml index 051b54143b..2fa61528a7 100644 --- a/.github/actions/nss/action.yml +++ b/.github/actions/nss/action.yml @@ -49,14 +49,14 @@ runs: # # - name: Checkout NSPR # if: env.BUILD_NSS == '1' - # uses: actions/checkout@v4 + # uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 # with: # repository: "nss-dev/nspr" # path: ${{ github.workspace }}/nspr # - name: Checkout NSS # if: env.BUILD_NSS == '1' - # uses: actions/checkout@v4 + # uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 # with: # repository: "nss-dev/nss" # path: ${{ github.workspace }}/nss diff --git a/.github/actions/pr-comment-data-export/action.yml b/.github/actions/pr-comment-data-export/action.yml index 7ec57a9b9e..452568e54d 100644 --- a/.github/actions/pr-comment-data-export/action.yml +++ b/.github/actions/pr-comment-data-export/action.yml @@ -31,7 +31,7 @@ runs: echo "${{ inputs.log-url }}" > comment-data/log-url fi - if: github.event_name == 'pull_request' - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 with: name: ${{ inputs.name }} path: comment-data diff --git a/.github/actions/quic-interop-runner/action.yml b/.github/actions/quic-interop-runner/action.yml index e987480d24..ec4db19fe1 100644 --- a/.github/actions/quic-interop-runner/action.yml +++ b/.github/actions/quic-interop-runner/action.yml @@ -24,7 +24,7 @@ runs: using: "composite" steps: - name: Checkout quic-interop/quic-interop-runner repository - uses: actions/checkout@v4 + uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 with: repository: 'quic-interop/quic-interop-runner' path: 'quic-interop-runner' @@ -74,7 +74,7 @@ runs: python run.py $ARGS 2>&1 | tee ../summary.txt || true shell: bash - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 id: upload-logs with: name: '${{ inputs.client }} vs. ${{ inputs.server }} logs' diff --git a/.github/workflows/actionlint.yml b/.github/workflows/actionlint.yml index c0e6de01c0..b258454518 100644 --- a/.github/workflows/actionlint.yml +++ b/.github/workflows/actionlint.yml @@ -12,6 +12,8 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref_name }} cancel-in-progress: true +permissions: read-all + jobs: actionlint: runs-on: ubuntu-latest @@ -19,7 +21,7 @@ jobs: run: shell: bash steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - name: Download actionlint id: get_actionlint run: bash <(curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash) diff --git a/.github/workflows/bench-comment.yml b/.github/workflows/bench-comment.yml index f89d223059..dce7e25f5f 100644 --- a/.github/workflows/bench-comment.yml +++ b/.github/workflows/bench-comment.yml @@ -12,6 +12,8 @@ on: types: - completed +permissions: read-all + jobs: comment: permissions: @@ -21,7 +23,7 @@ jobs: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - uses: ./.github/actions/pr-comment with: name: bench diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 5df8bcfd91..232ad4c6f1 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -14,6 +14,8 @@ env: RUSTFLAGS: -C link-arg=-fuse-ld=lld -C link-arg=-Wl,--no-rosegment, -C force-frame-pointers=yes PERF_OPT: record -F997 --call-graph fp -g +permissions: read-all + jobs: bench: name: Benchmark @@ -24,10 +26,10 @@ jobs: steps: - name: Checkout neqo - uses: actions/checkout@v4 + uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - name: Checkout msquic - uses: actions/checkout@v4 + uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 with: repository: microsoft/msquic ref: main @@ -60,7 +62,7 @@ jobs: - name: Download cached main-branch results id: criterion-cache - uses: actions/cache/restore@v4 + uses: actions/cache/restore@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 with: path: ./target/criterion key: criterion-${{ runner.name }}-${{ github.sha }} @@ -220,14 +222,14 @@ jobs: - name: Cache main-branch results if: github.ref == 'refs/heads/main' - uses: actions/cache/save@v4 + uses: actions/cache/save@0c45773b623bea8c8e75f6c82b208c3cf94ea4f9 # v4.0.2 with: path: ./target/criterion key: criterion-${{ runner.name }}-${{ github.sha }} - name: Export perf data id: export - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 with: name: ${{ github.event.repository.name }}-${{ github.sha }} path: | diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 51376bf79d..00971a6ac4 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -15,6 +15,8 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref_name }} cancel-in-progress: true +permissions: read-all + jobs: check: name: Build & test @@ -39,7 +41,7 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - name: Install dependencies (Linux) if: runner.os == 'Linux' @@ -73,6 +75,8 @@ jobs: - name: Set up MSVC build environment (Windows) if: runner.os == 'Windows' uses: ilammy/msvc-dev-cmd@v1 + # TODO: Would like to pin this, but the Mozilla org allowlist requires "ilammy/msvc-dev-cmd@v1*" + # uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0 - name: Set up NSS/NSPR build environment (Windows) if: runner.os == 'Windows' @@ -147,7 +151,7 @@ jobs: if: success() || failure() - name: Upload coverage reports to Codecov - uses: codecov/codecov-action@v4 + uses: codecov/codecov-action@84508663e988701840491b86de86b666e8a86bed # v4.3.0 with: file: lcov.info fail_ci_if_error: false diff --git a/.github/workflows/mutants.yml b/.github/workflows/mutants.yml index 4db2e4b925..1837749da8 100644 --- a/.github/workflows/mutants.yml +++ b/.github/workflows/mutants.yml @@ -11,11 +11,13 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref_name }} cancel-in-progress: true +permissions: read-all + jobs: mutants: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 with: fetch-depth: 0 @@ -61,7 +63,7 @@ jobs: } > "$GITHUB_STEP_SUMMARY" - name: Archive mutants.out - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 if: always() with: name: mutants.out diff --git a/.github/workflows/qns-comment.yml b/.github/workflows/qns-comment.yml index 8bbc4a7d32..f27d4904a1 100644 --- a/.github/workflows/qns-comment.yml +++ b/.github/workflows/qns-comment.yml @@ -12,6 +12,8 @@ on: types: - completed +permissions: read-all + jobs: comment: permissions: @@ -20,7 +22,7 @@ jobs: if: | github.event.workflow_run.event == 'pull_request' steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - uses: ./.github/actions/pr-comment with: name: qns diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index f60958eeee..5efd14c2ce 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -12,6 +12,8 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref_name }} cancel-in-progress: true +permissions: read-all + env: LATEST: neqo-latest DELIM: ' vs. ' @@ -25,15 +27,15 @@ jobs: permissions: packages: write steps: - - uses: docker/setup-qemu-action@v3 - - uses: docker/setup-buildx-action@v3 - - uses: docker/login-action@v3 + - uses: docker/setup-qemu-action@68827325e0b33c7199eb31dd4e31fbe9023e06e3 # v3.0.0 + - uses: docker/setup-buildx-action@d70bba72b1f3fd22344832f00baa16ece964efeb # v3.3.0 + - uses: docker/login-action@e92390c5fb421da1463c202d546fed0ec5c39f20 # v3.1.0 with: registry: ghcr.io username: ${{ github.actor }} password: ${{ github.token }} - - uses: docker/metadata-action@v5 + - uses: docker/metadata-action@8e5442c4ef9f78752691e2d8f8d19755c6f78e81 # v5.5.1 id: meta with: images: ghcr.io/${{ github.repository }}-qns @@ -46,7 +48,7 @@ jobs: # set latest tag for default branch type=raw,value=latest,enable={{is_default_branch}} - - uses: docker/build-push-action@v5 + - uses: docker/build-push-action@2cdde995de11925a030ce8070c3d77a52ffcf1c0 # v5.3.0 if: github.event_name != 'pull_request' with: push: true @@ -57,7 +59,7 @@ jobs: cache-to: type=gha,mode=max platforms: 'linux/amd64, linux/arm64' - - uses: docker/build-push-action@v5 + - uses: docker/build-push-action@2cdde995de11925a030ce8070c3d77a52ffcf1c0 # v5.3.0 if: github.event_name == 'pull_request' id: docker_build_and_push with: @@ -69,7 +71,7 @@ jobs: platforms: 'linux/amd64' outputs: type=docker,dest=/tmp/${{ env.LATEST }}.tar - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 if: github.event_name == 'pull_request' with: name: '${{ env.LATEST }} Docker image' @@ -155,7 +157,7 @@ jobs: needs: run-qns runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - uses: actions/download-artifact@v4 with: pattern: '*results' diff --git a/qns/Dockerfile b/qns/Dockerfile index cdb192f203..c4bbd2a79e 100644 --- a/qns/Dockerfile +++ b/qns/Dockerfile @@ -1,4 +1,4 @@ -FROM martenseemann/quic-network-simulator-endpoint:latest AS buildimage +FROM martenseemann/quic-network-simulator-endpoint@sha256:12596544531465e77bdede50dd1e85b2c46c00f1634b3445eed277ca177666db AS buildimage RUN apt-get update && apt-get install -y --no-install-recommends \ curl git mercurial coreutils \ @@ -13,8 +13,15 @@ ENV RUSTUP_HOME=/usr/local/rustup \ CARGO_HOME=/usr/local/cargo \ PATH=/usr/local/cargo/bin:$PATH -RUN curl https://sh.rustup.rs -sSf | \ - sh -s -- -y -q --no-modify-path --profile minimal --default-toolchain $RUST_VERSION +ADD --checksum=sha256:a3d541a5484c8fa2f1c21478a6f6c505a778d473c21d60a18a4df5185d320ef8 \ + https://static.rust-lang.org/rustup/dist/x86_64-unknown-linux-gnu/rustup-init x86_64_rustup + +ADD --checksum=sha256:76cd420cb8a82e540025c5f97bda3c65ceb0b0661d5843e6ef177479813b0367 \ + https://static.rust-lang.org/rustup/dist/aarch64-unknown-linux-gnu/rustup-init aarch64_rustup + +RUN mv $(uname -m)_rustup rustup-init && \ + chmod +x rustup-init && \ + ./rustup-init -y -q --no-modify-path --profile minimal --default-toolchain $RUST_VERSION ENV NSS_DIR=/nss \ NSPR_DIR=/nspr \ @@ -35,7 +42,7 @@ RUN set -eux; \ # Copy only binaries to the final image to keep it small. -FROM martenseemann/quic-network-simulator-endpoint:latest +FROM martenseemann/quic-network-simulator-endpoint@sha256:12596544531465e77bdede50dd1e85b2c46c00f1634b3445eed277ca177666db ENV LD_LIBRARY_PATH=/neqo/lib COPY --from=buildimage /neqo/target/release/neqo-client /neqo/target/release/neqo-server /neqo/bin/ From 17c5895a37b5247c6532e5e0d433e7aad7c7e732 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 25 Apr 2024 09:27:11 +0200 Subject: [PATCH 02/16] fix(bin): consider env when no verbosity flag is set (#1848) * fix(bin): consider env when no verbosity flag is set There are two ways to specify the maximum log level of `neqo-server` and `neqo-client`. Via the `RUST_LOG` environment variable and since https://github.com/mozilla/neqo/pull/1692 via the verbosity flags (e.g. `-v`). Previously, if no verbosity flag was provided, `INFO` was set as the maximum log level, the `RUST_LOG` environment variable was simply ignored. With this commit, if no flag is set, the `RUST_LOG` environment variable is considered, and only then the `env_logger` crate default value (`ERROR`) is used. In other words, the precedence order now is: 1. command line flags (e.g. `-v`) 2. `RUST_LOG` environment variable 3. default `ERROR` level * clippy * Move `verbose` into `SharedArgs` --- neqo-bin/src/client/mod.rs | 11 ++++++----- neqo-bin/src/lib.rs | 4 ++++ neqo-bin/src/server/mod.rs | 11 ++++++----- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/neqo-bin/src/client/mod.rs b/neqo-bin/src/client/mod.rs index fd77785c20..6d0b5dad6f 100644 --- a/neqo-bin/src/client/mod.rs +++ b/neqo-bin/src/client/mod.rs @@ -104,9 +104,6 @@ type Res = Result; #[command(author, version, about, long_about = None)] #[allow(clippy::struct_excessive_bools)] // Not a good use of that lint. pub struct Args { - #[command(flatten)] - verbose: clap_verbosity_flag::Verbosity, - #[command(flatten)] shared: SharedArgs, @@ -180,7 +177,6 @@ impl Args { pub fn new(requests: &[u64]) -> Self { use std::str::FromStr; Self { - verbose: clap_verbosity_flag::Verbosity::::default(), shared: crate::SharedArgs::default(), urls: requests .iter() @@ -479,7 +475,12 @@ fn qlog_new(args: &Args, hostname: &str, cid: &ConnectionId) -> Res { } pub async fn client(mut args: Args) -> Res<()> { - neqo_common::log::init(Some(args.verbose.log_level_filter())); + neqo_common::log::init( + args.shared + .verbose + .as_ref() + .map(clap_verbosity_flag::Verbosity::log_level_filter), + ); init()?; args.update_for_tests(); diff --git a/neqo-bin/src/lib.rs b/neqo-bin/src/lib.rs index 380c56ddce..9b5de9e0c2 100644 --- a/neqo-bin/src/lib.rs +++ b/neqo-bin/src/lib.rs @@ -26,6 +26,9 @@ mod udp; #[derive(Debug, Parser)] pub struct SharedArgs { + #[command(flatten)] + verbose: Option, + #[arg(short = 'a', long, default_value = "h3")] /// ALPN labels to negotiate. /// @@ -66,6 +69,7 @@ pub struct SharedArgs { impl Default for SharedArgs { fn default() -> Self { Self { + verbose: None, alpn: "h3".into(), qlog_dir: None, max_table_size_encoder: 16384, diff --git a/neqo-bin/src/server/mod.rs b/neqo-bin/src/server/mod.rs index 78677e4402..df385119c2 100644 --- a/neqo-bin/src/server/mod.rs +++ b/neqo-bin/src/server/mod.rs @@ -98,9 +98,6 @@ type Res = Result; #[derive(Debug, Parser)] #[command(author, version, about, long_about = None)] pub struct Args { - #[command(flatten)] - verbose: clap_verbosity_flag::Verbosity, - #[command(flatten)] shared: SharedArgs, @@ -132,7 +129,6 @@ impl Default for Args { fn default() -> Self { use std::str::FromStr; Self { - verbose: clap_verbosity_flag::Verbosity::::default(), shared: crate::SharedArgs::default(), hosts: vec!["[::]:12345".to_string()], db: PathBuf::from_str("../test-fixture/db").unwrap(), @@ -587,7 +583,12 @@ enum Ready { pub async fn server(mut args: Args) -> Res<()> { const HQ_INTEROP: &str = "hq-interop"; - neqo_common::log::init(Some(args.verbose.log_level_filter())); + neqo_common::log::init( + args.shared + .verbose + .as_ref() + .map(clap_verbosity_flag::Verbosity::log_level_filter), + ); assert!(!args.key.is_empty(), "Need at least one key"); init_db(args.db.clone())?; From b18a614b36518fc23baf2c63dc461140216025cc Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Mon, 29 Apr 2024 11:26:37 +0300 Subject: [PATCH 03/16] chore: Fix clippy nightly warnings (#1853) --- neqo-crypto/src/aead_null.rs | 2 -- neqo-crypto/src/constants.rs | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/neqo-crypto/src/aead_null.rs b/neqo-crypto/src/aead_null.rs index 2d5656de73..6fcb72871f 100644 --- a/neqo-crypto/src/aead_null.rs +++ b/neqo-crypto/src/aead_null.rs @@ -4,8 +4,6 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -#![cfg(feature = "disable-encryption")] - use std::fmt; use crate::{ diff --git a/neqo-crypto/src/constants.rs b/neqo-crypto/src/constants.rs index 76db972290..daef3d3c56 100644 --- a/neqo-crypto/src/constants.rs +++ b/neqo-crypto/src/constants.rs @@ -27,7 +27,7 @@ pub const TLS_EPOCH_APPLICATION_DATA: Epoch = 3_u16; macro_rules! remap_enum { { $t:ident: $s:ty { $( $n:ident = $v:path ),+ $(,)? } } => { pub type $t = $s; - $( pub const $n: $t = $v as $t; )+ + $(#[allow(clippy::cast_possible_truncation)] pub const $n: $t = $v as $t; )+ }; { $t:ident: $s:ty => $e:ident { $( $n:ident = $v:ident ),+ $(,)? } } => { remap_enum!{ $t: $s { $( $n = $e::$v ),+ } } From 8f99c3d0a01241e2fb2a3cc72e42775c7e868475 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Mon, 29 Apr 2024 05:47:28 -0400 Subject: [PATCH 04/16] =?UTF-8?q?build:=20upgrade=20`indexmap`=201.9=20?= =?UTF-8?q?=E2=86=92=202.2=20(#1859)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * build: upgrade `indexmap` 1.9 → 2.2.6 * Wildcard to 2.2 and add pointer to rationale --------- Co-authored-by: Lars Eggert --- neqo-transport/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index 6095f3ac92..2abdbbfd95 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -14,7 +14,7 @@ workspace = true [dependencies] # Sync with https://searchfox.org/mozilla-central/source/Cargo.lock 2024-02-08 enum-map = { version = "2.7", default-features = false } -indexmap = { version = "1.9", default-features = false } +indexmap = { version = "2.2", default-features = false } # See https://github.com/mozilla/neqo/issues/1858 log = { workspace = true } neqo-common = { path = "../neqo-common" } neqo-crypto = { path = "../neqo-crypto" } From 52675c358c29a3a1c434050b93719cf88c86c61a Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 29 Apr 2024 17:25:38 +0200 Subject: [PATCH 05/16] fix(.github/qns): print to stdout instead of undefined $GROUP.md (#1860) The qns.yml workflow adds a heading to the GitHub comment linking to the quic-interop-runner repository. The heading is redirected to `$GROUP.md`. Though `$GROUP` is not defined in this scope. This commit changes the `echo` command to print to `stdout` directly. --- .github/workflows/qns.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 5efd14c2ce..8ffe186605 100644 --- a/.github/workflows/qns.yml +++ b/.github/workflows/qns.yml @@ -196,7 +196,7 @@ jobs: done { echo "### Failed Interop Tests" - echo "[QUIC Interop Runner](https://github.com/quic-interop/quic-interop-runner), *client* vs. *server*" >> "$GROUP.md" + 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 From d4b3e4c58edad21642c08a5fb826526359b9c32d Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 30 Apr 2024 13:28:20 +0300 Subject: [PATCH 06/16] ci: Pin more deps (#1855) Missed a few. See also #1843 --- .github/actions/pr-comment/action.yml | 4 ++-- .github/actions/quic-interop-runner/action.yml | 4 ++-- .github/actions/rust/action.yml | 6 +++--- .github/workflows/qns.yml | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/actions/pr-comment/action.yml b/.github/actions/pr-comment/action.yml index 75eb547562..6ee1e7d813 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@v4 + - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 with: run-id: ${{ github.event.workflow_run.id }} name: ${{ inputs.name }} @@ -32,7 +32,7 @@ runs: echo "[:arrow_down: Download logs]($(cat log-url))" >> contents fi - - uses: thollander/actions-comment-pull-request@v2 + - uses: thollander/actions-comment-pull-request@fabd468d3a1a0b97feee5f6b9e499eab0dd903f6 # v2.5.0 with: filePath: contents mode: ${{ inputs.mode }} diff --git a/.github/actions/quic-interop-runner/action.yml b/.github/actions/quic-interop-runner/action.yml index ec4db19fe1..3f5547d3c6 100644 --- a/.github/actions/quic-interop-runner/action.yml +++ b/.github/actions/quic-interop-runner/action.yml @@ -40,7 +40,7 @@ runs: sudo apt-get install -y --no-install-recommends tshark shell: bash - - uses: actions/setup-python@v5 + - uses: actions/setup-python@82c7e631bb3cdc910f68e0081d67478d79c6982d # v5.1.0 with: python-version: 3.8 cache: 'pip' @@ -88,7 +88,7 @@ runs: mv result.json.tmp result.json shell: bash - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 with: name: '${{ inputs.client }} vs. ${{ inputs.server }} results' path: | diff --git a/.github/actions/rust/action.yml b/.github/actions/rust/action.yml index 4b03b37b8d..b363738cca 100644 --- a/.github/actions/rust/action.yml +++ b/.github/actions/rust/action.yml @@ -13,7 +13,7 @@ runs: using: composite steps: - name: Install Rust - uses: dtolnay/rust-toolchain@master + uses: dtolnay/rust-toolchain@bb45937a053e097f8591208d8e74c90db1873d07 # master with: toolchain: ${{ inputs.version }} components: ${{ inputs.components }} @@ -35,7 +35,7 @@ runs: # sccache slows CI down, so we leave it disabled. # Leaving the steps below commented out, so we can re-evaluate enabling it later. # - name: Use sccache - # uses: mozilla-actions/sccache-action@v0.0.4 + # uses: mozilla-actions/sccache-action@2e7f9ec7921547d4b46598398ca573513895d0bd # v0.0.4 # - name: Enable sscache # shell: bash @@ -53,6 +53,6 @@ runs: # Ditto for rust-cache. # - name: Use Rust cache - # uses: Swatinem/rust-cache@v2 + # uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2.7.3 # with: # cache-all-crates: "true" diff --git a/.github/workflows/qns.yml b/.github/workflows/qns.yml index 8ffe186605..45ea31ffc5 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@v4 + - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 with: name: '${{ env.LATEST }} Docker image' path: /tmp @@ -142,7 +142,7 @@ jobs: echo "client=$(echo "$PAIR" | cut -d% -f1)" >> "$GITHUB_OUTPUT" echo "server=$(echo "$PAIR" | cut -d% -f2)" >> "$GITHUB_OUTPUT" - - uses: actions/checkout@v4 + - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 # TODO: Replace once https://github.com/quic-interop/quic-interop-runner/pull/356 is merged. - uses: ./.github/actions/quic-interop-runner @@ -158,7 +158,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@1d96c772d19495a3b5c517cd2bc0cb401ea0529f # v4.1.3 - - uses: actions/download-artifact@v4 + - uses: actions/download-artifact@9c19ed7fe5d278cd354c7dfd5d3b88589c7e2395 # v4.1.6 with: pattern: '*results' path: results From dcc88e323edfb5d2c4ad483bca0a93b89c5f22c2 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Tue, 30 Apr 2024 17:43:49 +0300 Subject: [PATCH 07/16] test: Make `test.sh` more verbose again (#1863) Might have been changed by #1848 --- test/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test.sh b/test/test.sh index dc02b2161c..2c22e9b6c8 100755 --- a/test/test.sh +++ b/test/test.sh @@ -17,7 +17,7 @@ cargo build --bin neqo-client --bin neqo-server addr=127.0.0.1 port=4433 path=/20000 -flags="--verbose --qlog-dir $tmp --use-old-http --alpn hq-interop --quic-version 1" +flags="--verbose --verbose --verbose --qlog-dir $tmp --use-old-http --alpn hq-interop --quic-version 1" if [ "$(uname -s)" != "Linux" ]; then iface=lo0 else From 14cafbaa7fa88434def2c1d19e932c08e00173f8 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 2 May 2024 08:40:04 +0300 Subject: [PATCH 08/16] 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 09/16] 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 10/16] 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 11/16] 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 12/16] 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 13/16] 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 14/16] 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 15/16] 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 16/16] 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)) ); }