cloudflare / cloudflare/quiche
tokio-quiche doesn't propagate handshake errors
- Dominant language
- Rust
- Stars
- 11.8k
- Forks
- 1.1k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 16
Description
When running `InitialQuicConnection.start(driver)` the handshake and resume is completed within. However, in my testing where mTLS fails due to incorrect host names (doesn't match Common Name in TLS Cert), the `.start(driver)` silently fails and what happens is my app assumes the quic handshake succeeded but it actually failed so it initializes the app logic and runs the tasks in them since there is no way to know if the handshake failed or not (to my knowledge).
To fix the issue, I implemented this in my app
```rust
async fn start_connection
(
initial_quic_connection: InitialQuicConnection,
driver: MyDriver
) -> io::Result
where
M: Metrics
{
let (q_conn, handshake) = initial_quic_connection.handshake(driver).await?;
metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
DefaultMetrics, //can be cloned if used within the crate
async move {
InitialQuicConnection::::resume(handshake)
}
);
Ok(q_conn)
}
```
which is taken from
```rust
pub fn start(self, app: A) -> QuicConnection {
let task_metrics = self.params.metrics.clone();
let (conn, handshake_fut) = Self::handshake_fut(self, app);
let fut = async move {
match handshake_fut.await {
Ok(running) => Self::resume(running),
Err(e) => {
log::error!("QUIC handshake failed in IQC::start"; "error" => e)
},
}
};
crate::metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
task_metrics,
fut,
);
conn
}
```
Why don't we create a new start function to support returning the result?
```rust
pub async fn start_with_result(self, app: A) -> io::Result {
let task_metrics = self.params.metrics.clone();
let result = self.handshake(app).await;
match result {
Ok((q_conn, handshake)) => {
crate::metrics::tokio_task::spawn_with_killswitch(
"quic_handshake_worker",
task_metrics,
async move {
Self::resume(handshake)
}
);
Ok(q_conn)
},
Err(err) => {
log::error!("QUIC handshake failed in IQC::start_with_result"; "error" => &err);
Err(err) // Pass it upward
}
}
}
```
This would propagate errors upward which can be handled by the user instead being silently logged while also maintaining the same logging behavior as `start()`.
Contributor guide
Research direction
Trace InitialQuicConnection::start, handshake, and resume, focusing on how the handshake future is spawned through spawn_with_killswitch. Compare the existing start flow with the proposed result-returning flow; done means handshake errors reach the caller while successful connections retain the current resume and logging behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100