Skip to content
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

Expose connection info in errors #337

Merged
merged 4 commits into from
Aug 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 57 additions & 1 deletion src/error.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
//! Types for error handling.

use std::{error::Error as StdError, fmt, io, sync::Arc};
use std::{error::Error as StdError, fmt, io, net::SocketAddr, sync::Arc};

use http::Response;
use once_cell::sync::OnceCell;

use crate::ResponseExt;

/// A non-exhaustive list of error types that can occur while sending an HTTP
/// request or receiving an HTTP response.
Expand Down Expand Up @@ -165,6 +170,8 @@ struct Inner {
kind: ErrorKind,
context: Option<String>,
source: Option<Box<dyn SourceError>>,
local_addr: OnceCell<SocketAddr>,
remote_addr: OnceCell<SocketAddr>,
}

impl Error {
Expand All @@ -186,9 +193,26 @@ impl Error {
kind,
context,
source: Some(Box::new(source)),
local_addr: OnceCell::new(),
remote_addr: OnceCell::new(),
}))
}

/// Create a new error from a given error kind and response.
pub(crate) fn with_response<B>(kind: ErrorKind, response: &Response<B>) -> Self {
let error = Self::from(kind);

if let Some(addr) = response.local_addr() {
let _ = error.0.local_addr.set(addr);
}

if let Some(addr) = response.remote_addr() {
let _ = error.0.remote_addr.set(addr);
}

error
}

/// Statically cast a given error into an Isahc error, converting if
/// necessary.
///
Expand Down Expand Up @@ -346,6 +370,34 @@ impl Error {
_ => false,
}
}

/// Get the local socket address of the last-used connection involved in
/// this error, if known.
///
/// If the request that caused this error failed to create a local socket
/// for connecting then this will return `None`.
pub fn local_addr(&self) -> Option<SocketAddr> {
self.0.local_addr.get().cloned()
}

/// Get the remote socket address of the last-used connection involved in
/// this error, if known.
///
/// If the request that caused this error failed to connect to any server,
/// then this will return `None`.
pub fn remote_addr(&self) -> Option<SocketAddr> {
self.0.remote_addr.get().cloned()
}

pub(crate) fn with_local_addr(self, addr: SocketAddr) -> Self {
let _ = self.0.local_addr.set(addr);
self
}

pub(crate) fn with_remote_addr(self, addr: SocketAddr) -> Self {
let _ = self.0.remote_addr.set(addr);
self
}
}

impl StdError for Error {
Expand All @@ -370,6 +422,8 @@ impl fmt::Debug for Error {
"source_type",
&self.0.source.as_ref().map(|e| e.type_name()),
)
.field("local_addr", &self.0.local_addr.get())
.field("remote_addr", &self.0.remote_addr.get())
.finish()
}
}
Expand All @@ -390,6 +444,8 @@ impl From<ErrorKind> for Error {
kind,
context: None,
source: None,
local_addr: OnceCell::new(),
remote_addr: OnceCell::new(),
}))
}
}
Expand Down
12 changes: 12 additions & 0 deletions src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,18 @@ impl RequestHandler {

/// Set the final result for this transfer.
pub(crate) fn set_result(&mut self, result: Result<(), Error>) {
let result = result.map_err(|mut e| {
if let Some(addr) = self.get_local_addr() {
e = e.with_local_addr(addr);
}

if let Some(addr) = self.get_primary_addr() {
e = e.with_remote_addr(addr);
}

e
});

if self.shared.result.set(result).is_err() {
tracing::debug!("attempted to set error multiple times");
}
Expand Down
7 changes: 5 additions & 2 deletions src/redirect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl Interceptor for RedirectInterceptor {
if let Some(location) = get_redirect_location(&effective_uri, &response) {
// If we've reached the limit, return an error as requested.
if redirect_count >= limit {
return Err(ErrorKind::TooManyRedirects.into());
return Err(Error::with_response(ErrorKind::TooManyRedirects, &response));
}

// Set referer header.
Expand Down Expand Up @@ -113,7 +113,10 @@ impl Interceptor for RedirectInterceptor {
// There's not really a good way of handling this gracefully, so
// we just return an error so that the user knows about it.
if !request_body.reset() {
return Err(ErrorKind::RequestBodyNotRewindable.into());
return Err(Error::with_response(
ErrorKind::RequestBodyNotRewindable,
&response,
));
}

// Update the request to point to the new URI.
Expand Down
4 changes: 2 additions & 2 deletions src/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl<R: Read> ReadResponseExt<R> for Response<R> {

#[cfg(feature = "text-decoding")]
fn text(&mut self) -> io::Result<String> {
crate::text::Decoder::for_response(&self).decode_reader(self.body_mut())
crate::text::Decoder::for_response(self).decode_reader(self.body_mut())
}

#[cfg(feature = "json")]
Expand Down Expand Up @@ -430,7 +430,7 @@ impl<R: AsyncRead + Unpin> AsyncReadResponseExt<R> for Response<R> {

#[cfg(feature = "text-decoding")]
fn text(&mut self) -> crate::text::TextFuture<'_, &mut R> {
crate::text::Decoder::for_response(&self).decode_reader_async(self.body_mut())
crate::text::Decoder::for_response(self).decode_reader_async(self.body_mut())
}

#[cfg(feature = "json")]
Expand Down
20 changes: 19 additions & 1 deletion tests/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ fn local_addr_returns_expected_address() {
}

#[test]
fn remote_addr_returns_expected_address_expected_address() {
fn remote_addr_returns_expected_address() {
let m = mock!();

let response = isahc::get(m.url()).unwrap();
Expand All @@ -43,6 +43,24 @@ fn remote_addr_returns_expected_address_expected_address() {
assert_eq!(response.remote_addr(), Some(m.addr()));
}

#[test]
fn local_and_remote_addr_returns_expected_addresses_on_error() {
let server = TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap();
let addr = server.local_addr().unwrap();

thread::spawn(move || {
let (mut client, _) = server.accept().unwrap();
client.write_all(b"foobar").unwrap();
client.flush().unwrap();
});

let error = isahc::get(format!("http://localhost:{}", addr.port())).unwrap_err();

assert_eq!(error.remote_addr(), Some(addr));
assert_eq!(error.local_addr().unwrap().ip(), Ipv4Addr::LOCALHOST);
assert!(error.local_addr().unwrap().port() > 0);
}

#[test]
fn ipv4_only_will_not_connect_to_ipv6() {
if !is_ipv6_supported() {
Expand Down
19 changes: 10 additions & 9 deletions tests/redirects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ use isahc::{config::RedirectPolicy, prelude::*, Body, HttpClient, Request};
use test_case::test_case;
use testserver::mock;

#[macro_use]
mod utils;

#[test]
fn response_301_no_follow() {
let m = mock! {
Expand Down Expand Up @@ -216,13 +213,15 @@ fn redirect_non_rewindable_body_returns_error() {
// Create a streaming body of unknown size.
let upload_stream = Body::from_reader(Body::from_bytes_static(b"hello world"));

let result = Request::post(m1.url())
let error = Request::post(m1.url())
.redirect_policy(RedirectPolicy::Follow)
.body(upload_stream)
.unwrap()
.send();
.send()
.unwrap_err();

assert_matches!(result, Err(e) if e == isahc::error::ErrorKind::RequestBodyNotRewindable);
assert_eq!(error, isahc::error::ErrorKind::RequestBodyNotRewindable);
assert_eq!(error.remote_addr(), Some(m1.addr()));
assert_eq!(m1.request().method, "POST");
}

Expand All @@ -235,14 +234,16 @@ fn redirect_limit_is_respected() {
}
};

let result = Request::get(m.url())
let error = Request::get(m.url())
.redirect_policy(RedirectPolicy::Limit(5))
.body(())
.unwrap()
.send();
.send()
.unwrap_err();

// Request should error with too many redirects.
assert_matches!(result, Err(e) if e == isahc::error::ErrorKind::TooManyRedirects);
assert_eq!(error, isahc::error::ErrorKind::TooManyRedirects);
assert_eq!(error.remote_addr(), Some(m.addr()));

// After request (limit + 1) that returns a redirect should error.
assert_eq!(m.requests().len(), 6);
Expand Down