Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLPMTUD test fails to panic in release mode #2090

Open
gretchenfrage opened this issue Dec 15, 2024 · 2 comments · May be fixed by #2091
Open

PLPMTUD test fails to panic in release mode #2090

gretchenfrage opened this issue Dec 15, 2024 · 2 comments · May be fixed by #2091

Comments

@gretchenfrage
Copy link
Contributor

gretchenfrage commented Dec 15, 2024

On current main (73545b6), running:

cargo test --release -p quinn-proto -- connection::mtud::tests::mtu_discovery_with_peer_max_udp_payload_size_after_search_panics

Gives the following failure:

---- connection::mtud::tests::mtu_discovery_with_peer_max_udp_payload_size_after_search_panics stdout ----
note: test did not panic as expected

This only occurs if the --release flag is passed into the test run. The cause of it is quinn-proto/src/connection/mtud.rs:88 being a debug assertion:

// MTUD is only active after the connection has been fully established, so it is
// guaranteed we will receive the peer's transport parameters before we start probing
debug_assert!(matches!(state.phase, Phase::Initial));

Related to #1510

@Ralith
Copy link
Collaborator

Ralith commented Dec 15, 2024

I guess we could gate that whole test on debug_assertions?

@gretchenfrage
Copy link
Contributor Author

Option 1: Enable the panic

diff --git a/quinn-proto/src/connection/mtud.rs b/quinn-proto/src/connection/mtud.rs
index be1fe7ee..6e48b5a9 100644
--- a/quinn-proto/src/connection/mtud.rs
+++ b/quinn-proto/src/connection/mtud.rs
@@ -85,7 +85,12 @@ impl MtuDiscovery {
         if let Some(state) = self.state.as_mut() {
             // MTUD is only active after the connection has been fully established, so it is
             // guaranteed we will receive the peer's transport parameters before we start probing
-            debug_assert!(matches!(state.phase, Phase::Initial));
+            if cfg!(any(debug_assertions, test)) {
+                // The test mtu_discovery_with_peer_max_udp_payload_size_after_search_panics
+                // asserts that this panics, so run the assertion even when testing in a release
+                // build
+                assert!(matches!(state.phase, Phase::Initial));
+            }
             state.peer_max_udp_payload_size = peer_max_udp_payload_size;
         }
     }

Option 2: Disable the test

diff --git a/quinn-proto/src/connection/mtud.rs b/quinn-proto/src/connection/mtud.rs
index be1fe7ee..a20f15d8 100644
--- a/quinn-proto/src/connection/mtud.rs
+++ b/quinn-proto/src/connection/mtud.rs
@@ -731,6 +731,7 @@ mod tests {
         assert!(completed(&mtud));
     }
 
+    #[cfg(debug_assertions)]
     #[test]
     #[should_panic]
     fn mtu_discovery_with_peer_max_udp_payload_size_after_search_panics() {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants