Skip to content

correctly expose HttpError through RetryError::source #580

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.

Sign up for GitHub

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

Merged
alamb merged 3 commits into from
Dec 18, 2025
Merged

correctly expose HttpError through RetryError::source #580

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1845379
correctly expose HttpError through RetryError::source
carlsverre Dec 16, 2025
6c8d9ea
Update src/client/retry.rs
carlsverre Dec 16, 2025
a79d2e9
break in loop
carlsverre Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/client/retry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ pub enum RequestError {
#[error("Server returned error response: {body}")]
Response { status: StatusCode, body: String },

#[error(transparent)]
// We need to use `{0}` here rather than `error(transparent)` to ensure that
// `Error::source` returns `HttpError` rather than the underlying
// `reqwest::Error` which would happen with `error(transparent)`.
#[error("{0}")]
Copy link
Contributor

@alamb alamb Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this change confusing given the docs on https://docs.rs/thiserror/latest/thiserror/#derives

Errors may use error(transparent) to forward the source and Display methods straight through to an underlying error without adding an additional message. This would be appropriate for enums that need an “anything else” variant.

Specifically the documentation mostly discusses the display implications.

However, I verified that the newly added test fails without this change:

thread 'client::retry::tests::test_retry' (1398936) panicked at src/client/retry.rs:745:9:
HttpError not found in source chain

So in summary nice 🕵️ work @carlsverre (👋 btw -- it is great to see you on github)

Copy link
Contributor Author

@carlsverre carlsverre Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, perhaps the thiserror docs could be a bit better. I think it's somewhat of a footgun that source is also transparently forwarded, which effectively means the embedded #[from] Foo is no longer accessible via recursively searching source.

Nice to see you too!

Http(#[from] HttpError),
}

Expand Down Expand Up @@ -511,7 +514,7 @@ mod tests {
use crate::RetryConfig;
use crate::client::mock_server::MockServer;
use crate::client::retry::{RequestError, RetryContext, RetryExt, body_contains_error};
use crate::client::{HttpClient, HttpResponse};
use crate::client::{HttpClient, HttpError, HttpErrorKind, HttpResponse};
use http::StatusCode;
use hyper::Response;
use hyper::header::LOCATION;
Expand Down Expand Up @@ -731,6 +734,17 @@ mod tests {
e.source().unwrap().to_string(),
"HTTP error: error sending request",
);
// the HttpError type needs to be directly accessible in the source chain
let mut err: &dyn Error = &e;
let mut found = false;
while let Some(source) = err.source() {
err = source;
if let Some(err) = err.downcast_ref::<HttpError>() {
found = true;
assert_eq!(err.kind(), HttpErrorKind::Request);
}
}
assert!(found, "HttpError not found in source chain");

// Retries on client timeout
mock.push_async_fn(|_| async move {
Expand Down