refactor(client): replace mio with tokio

[mxinden]

• Use tokio instead of mio.
• Refactor neqo-client to be more consistent with neqo-server.
  • Introduce read_datagram and ready.
  • Introduce ClientRunner and old::ClientRunner (consistent with ServersRunner).
• Fold handle_test into client (now ClientRunner::new).

---

Companion to #1581.

See #1535 for details.

With the above additional refactorings the diff is rather difficult to read. Let me know if you would prefer separate pull requests instead.

[mxinden]

Running the http3 QUIC interop test, there is a race condition. Two AuthenticationNeeded events are enqueued.

The neqo-client handler handles both events.

neqo/neqo-client/src/main.rs

Lines 727 to 729 in 9489511

	Http3ClientEvent::AuthenticationNeeded => {
	client.authenticated(AuthenticationStatus::Ok, Instant::now());
	}

On the first call to client.authenticated *self.auth_required = false; is set.

neqo/neqo-crypto/src/agent.rs

Lines 634 to 643 in 9489511

	/// Call this function to mark the peer as authenticated.
	///
	/// # Panics
	///
	/// If the handshake doesn't need to be authenticated.
	pub fn authenticated(&mut self, status: AuthenticationStatus) {
	assert!(self.state.authentication_needed());
	*self.auth_required = false;
	self.state = HandshakeState::Authenticated(status.into());
	}

On the second call to client.authenticated the assert panics assert!(self.state.authentication_needed());.

---

The above patch makes sure there is only ever a single AuthenticationNeeded event in the queue by removing any existing ones. Is this a valid patch? Does this change in ordering violate any invariants, i.e. does anything depend on the original order?

As an alternative, one could only call client.authenticated on not-yet-authenticated connections.

Alternative suggestions?

[martinthomson]

We probably need to avoid generating that event multiple times. I see that we generate that in response to being in the AuthenticationPending state on the TLS connection. The idea must have been that the state would not be asserted multiple times (that is, we would block until someone called authenticated() without calling handshake() again). Either that or we would de-duplicate the event successfully.

That's clearly not the case, so we're generating multiple events. I don't see an obvious fix for this other than just tracking the state. Try this:

diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs
index 2de38841..1141765f 100644
--- a/neqo-transport/src/connection/mod.rs
+++ b/neqo-transport/src/connection/mod.rs
@@ -2578,24 +2578,30 @@ impl Connection {

     fn handshake(
         &mut self,
         now: Instant,
         packet_version: Version,
         space: PacketNumberSpace,
         data: Option<&[u8]>,
     ) -> Res<()> {
         qtrace!([self], "Handshake space={} data={:0x?}", space, data);

+        let was_authentication_pending =
+            self.crypto.tls.state() == HandshakeState::AuthenticationPending;
         let try_update = data.is_some();
         match self.crypto.handshake(now, space, data)? {
             HandshakeState::Authenticated(_) | HandshakeState::InProgress => (),
-            HandshakeState::AuthenticationPending => self.events.authentication_needed(),
+            HandshakeState::AuthenticationPending => {
+                if !was_authentication_pending {
+                    self.events.authentication_needed()
+                }
+            }
             HandshakeState::EchFallbackAuthenticationPending(public_name) => self
                 .events
                 .ech_fallback_authentication_needed(public_name.clone()),
             HandshakeState::Complete(_) => {
                 if !self.state.connected() {
                     self.set_connected(now)?;
                 }
             }
             _ => {
                 unreachable!("Crypto state should not be new or failed after successful handshake")

Of course, that will need a test.

[codecov-commenter]

Codecov Report

Attention: 219 lines in your changes are missing coverage. Please review.

Comparison is base (9a394e9) 87.33% compared to head (9434e17) 87.33%.

Files	Patch %	Lines
neqo-client/src/main.rs	0.00%	219 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1612      +/-   ##
==========================================
- Coverage   87.33%   87.33%   -0.01%     
==========================================
  Files         124      124              
  Lines       39276    39288      +12     
==========================================
+ Hits        34302    34312      +10     
- Misses       4974     4976       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mxinden]

Thank you @martinthomson. I reverted my hot-fix, I added your patch and a test. Would you mind taking another look?

All QUIC interop tests, apart from the always failing amplification test, succeed now.

[mxinden]

Based on crypto_frame_split. Let me know what you think.

[mxinden]

Thank you Martin. Your patch suggestion helped a lot.

[martinthomson]

OK, after checking here, I think we're OK, but this is a tiny bit brittle in the sense that it depends on having the server certificate small enough to fit in the first packet, but large enough to force the CertificateVerify into the next packet. I've added commentary to that effect.

[mxinden]

Agreed on this being brittle. Thanks for the additions.