Skip to content

Commit a01e6b2

Browse files
authored
fix(client): improve client errors details if available (#184)
Fix a race condition in the legacy HTTP client's connection setup where connection errors (e.g., TLS failures, unexpected server responses) were discarded, resulting in vague ChannelClosed errors. seanmonstar/reqwest#2649
1 parent fc1d699 commit a01e6b2

File tree

2 files changed

+507
-5
lines changed

2 files changed

+507
-5
lines changed

src/client/legacy/client.rs

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -577,22 +577,89 @@ where
577577
panic!("http2 feature is not enabled");
578578
} else {
579579
#[cfg(feature = "http1")] {
580+
// Perform the HTTP/1.1 handshake on the provided I/O stream.
581+
// Uses the h1_builder to establish a connection, returning a sender (tx) for requests
582+
// and a connection task (conn) that manages the connection lifecycle.
580583
let (mut tx, conn) =
581-
h1_builder.handshake(io).await.map_err(Error::tx)?;
584+
h1_builder.handshake(io).await.map_err(crate::client::legacy::client::Error::tx)?;
582585

586+
// Log that the HTTP/1.1 handshake has completed successfully.
587+
// This indicates the connection is established and ready for request processing.
583588
trace!(
584589
"http1 handshake complete, spawning background dispatcher task"
585590
);
591+
// Create a oneshot channel to communicate errors from the connection task.
592+
// err_tx sends errors from the connection task, and err_rx receives them
593+
// to correlate connection failures with request readiness errors.
594+
let (err_tx, err_rx) = tokio::sync::oneshot::channel();
595+
// Spawn the connection task in the background using the executor.
596+
// The task manages the HTTP/1.1 connection, including upgrades (e.g., WebSocket).
597+
// Errors are sent via err_tx to ensure they can be checked if the sender (tx) fails.
586598
executor.execute(
587599
conn.with_upgrades()
588-
.map_err(|e| debug!("client connection error: {}", e))
600+
.map_err(|e| {
601+
// Log the connection error at debug level for diagnostic purposes.
602+
debug!("client connection error: {:?}", e);
603+
// Log that the error is being sent to the error channel.
604+
trace!("sending connection error to error channel");
605+
// Send the error via the oneshot channel, ignoring send failures
606+
// (e.g., if the receiver is dropped, which is handled later).
607+
let _ =err_tx.send(e);
608+
})
589609
.map(|_| ()),
590610
);
591-
611+
// Log that the client is waiting for the connection to be ready.
612+
// Readiness indicates the sender (tx) can accept a request without blocking.
613+
trace!("waiting for connection to be ready");
614+
// Check if the sender is ready to accept a request.
615+
// This ensures the connection is fully established before proceeding.
616+
// aka:
592617
// Wait for 'conn' to ready up before we
593618
// declare this tx as usable
594-
tx.ready().await.map_err(Error::tx)?;
595-
PoolTx::Http1(tx)
619+
match tx.ready().await {
620+
// If ready, the connection is usable for sending requests.
621+
Ok(_) => {
622+
// Log that the connection is ready for use.
623+
trace!("connection is ready");
624+
// Drop the error receiver, as it’s no longer needed since the sender is ready.
625+
// This prevents waiting for errors that won’t occur in a successful case.
626+
drop(err_rx);
627+
// Wrap the sender in PoolTx::Http1 for use in the connection pool.
628+
PoolTx::Http1(tx)
629+
}
630+
// If the sender fails with a closed channel error, check for a specific connection error.
631+
// This distinguishes between a vague ChannelClosed error and an actual connection failure.
632+
Err(e) if e.is_closed() => {
633+
// Log that the channel is closed, indicating a potential connection issue.
634+
trace!("connection channel closed, checking for connection error");
635+
// Check the oneshot channel for a specific error from the connection task.
636+
match err_rx.await {
637+
// If an error was received, it’s a specific connection failure.
638+
Ok(err) => {
639+
// Log the specific connection error for diagnostics.
640+
trace!("received connection error: {:?}", err);
641+
// Return the error wrapped in Error::tx to propagate it.
642+
return Err(crate::client::legacy::client::Error::tx(err));
643+
}
644+
// If the error channel is closed, no specific error was sent.
645+
// Fall back to the vague ChannelClosed error.
646+
Err(_) => {
647+
// Log that the error channel is closed, indicating no specific error.
648+
trace!("error channel closed, returning the vague ChannelClosed error");
649+
// Return the original error wrapped in Error::tx.
650+
return Err(crate::client::legacy::client::Error::tx(e));
651+
}
652+
}
653+
}
654+
// For other errors (e.g., timeout, I/O issues), propagate them directly.
655+
// These are not ChannelClosed errors and don’t require error channel checks.
656+
Err(e) => {
657+
// Log the specific readiness failure for diagnostics.
658+
trace!("connection readiness failed: {:?}", e);
659+
// Return the error wrapped in Error::tx to propagate it.
660+
return Err(crate::client::legacy::client::Error::tx(e));
661+
}
662+
}
596663
}
597664
#[cfg(not(feature = "http1"))] {
598665
panic!("http1 feature is not enabled");

0 commit comments

Comments
 (0)