-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
fix(ext/fetch): include URL and error details on fetch failures #24910
fix(ext/fetch): include URL and error details on fetch failures #24910
Conversation
9f7cb28
to
47dbb42
Compare
#[derive(Debug)] | ||
pub struct ClientSendError { | ||
uri: Uri, | ||
source: hyper_util::client::legacy::Error, | ||
} | ||
|
||
impl ClientSendError { | ||
pub fn is_connect_error(&self) -> bool { | ||
self.source.is_connect() | ||
} | ||
} | ||
|
||
impl std::fmt::Display for ClientSendError { | ||
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
write!( | ||
f, | ||
"error sending request for url ({uri}): {source}", | ||
uri = self.uri, | ||
// NOTE: we can use `std::error::Report` instead once it's stabilized. | ||
source = error_reporter::Report::new(&self.source), | ||
) | ||
} | ||
} | ||
|
||
impl std::error::Error for ClientSendError { | ||
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||
Some(&self.source) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reviewers: this custom error type is needed because we need to preserve the original error so we can later access with std::error::Error::source
to get hyper::Error
, while having to adjust the error message format using error_reporter::Report
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, not stoked on additional crate, but it seems simple enough.
Yeah I thought the same, but the crate |
This commit improves error messages that
fetch
generates on failure.Fixes #24835