From daf304c97312f640842b4c8980883dd4d61446e8 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Wed, 11 Aug 2021 00:00:23 -0500 Subject: [PATCH 1/4] Add connection info to errors --- src/error.rs | 30 +++++++++++++++++++++++++++++- src/handler.rs | 8 ++++++++ tests/net.rs | 18 +++++++++++++++++- 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index 31dea158..066b0ce8 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,6 +1,8 @@ //! 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 once_cell::sync::OnceCell; /// A non-exhaustive list of error types that can occur while sending an HTTP /// request or receiving an HTTP response. @@ -165,6 +167,7 @@ struct Inner { kind: ErrorKind, context: Option, source: Option>, + remote_addr: OnceCell, } impl Error { @@ -186,6 +189,7 @@ impl Error { kind, context, source: Some(Box::new(source)), + remote_addr: OnceCell::new(), })) } @@ -346,6 +350,28 @@ impl Error { _ => false, } } + + /// 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 be `None`. + /// + /// # Addresses and proxies + /// + /// The address returned by this method is the IP address and port that the + /// client _connected to_ and not necessarily the real address of the origin + /// server. Forward and reverse proxies between the caller and the server + /// can cause the address to be returned to reflect the address of the + /// nearest proxy rather than the server. + pub fn remote_addr(&self) -> Option { + self.0.remote_addr.get().cloned() + } + + pub(crate) fn with_remote_addr(self, addr: SocketAddr) -> Self { + let _ = self.0.remote_addr.set(addr); + self + } } impl StdError for Error { @@ -370,6 +396,7 @@ impl fmt::Debug for Error { "source_type", &self.0.source.as_ref().map(|e| e.type_name()), ) + .field("remote_addr", &self.0.remote_addr.get()) .finish() } } @@ -390,6 +417,7 @@ impl From for Error { kind, context: None, source: None, + remote_addr: OnceCell::new(), })) } } diff --git a/src/handler.rs b/src/handler.rs index 259e0162..cc38ae48 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -210,6 +210,14 @@ impl RequestHandler { /// Set the final result for this transfer. pub(crate) fn set_result(&mut self, result: Result<(), Error>) { + let result = result.map_err(|e| { + if let Some(addr) = self.get_primary_addr() { + e.with_remote_addr(addr) + } else { + e + } + }); + if self.shared.result.set(result).is_err() { tracing::debug!("attempted to set error multiple times"); } diff --git a/tests/net.rs b/tests/net.rs index 8b3a1ebf..635886df 100644 --- a/tests/net.rs +++ b/tests/net.rs @@ -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(); @@ -43,6 +43,22 @@ fn remote_addr_returns_expected_address_expected_address() { assert_eq!(response.remote_addr(), Some(m.addr())); } +#[test] +fn remote_addr_returns_expected_address_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)); +} + #[test] fn ipv4_only_will_not_connect_to_ipv6() { if !is_ipv6_supported() { From 58161d7cfbc83084b20847f54058b1161d4c0961 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Wed, 11 Aug 2021 23:45:55 -0500 Subject: [PATCH 2/4] Add local_addr to error as well --- src/error.rs | 46 +++++++++++++++++++++++++++++++++++++--------- src/handler.rs | 12 ++++++++---- src/redirect.rs | 4 ++-- tests/net.rs | 4 +++- tests/redirects.rs | 19 ++++++++++--------- 5 files changed, 60 insertions(+), 25 deletions(-) diff --git a/src/error.rs b/src/error.rs index 066b0ce8..0af51bf5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -2,8 +2,11 @@ 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. /// @@ -167,6 +170,7 @@ struct Inner { kind: ErrorKind, context: Option, source: Option>, + local_addr: OnceCell, remote_addr: OnceCell, } @@ -189,10 +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(kind: ErrorKind, response: &Response) -> 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. /// @@ -351,23 +371,30 @@ impl Error { } } + /// 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 { + 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 be `None`. - /// - /// # Addresses and proxies - /// - /// The address returned by this method is the IP address and port that the - /// client _connected to_ and not necessarily the real address of the origin - /// server. Forward and reverse proxies between the caller and the server - /// can cause the address to be returned to reflect the address of the - /// nearest proxy rather than the server. + /// then this will return `None`. pub fn remote_addr(&self) -> Option { 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 @@ -417,6 +444,7 @@ impl From for Error { kind, context: None, source: None, + local_addr: OnceCell::new(), remote_addr: OnceCell::new(), })) } diff --git a/src/handler.rs b/src/handler.rs index cc38ae48..cd02481c 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -210,12 +210,16 @@ impl RequestHandler { /// Set the final result for this transfer. pub(crate) fn set_result(&mut self, result: Result<(), Error>) { - let result = result.map_err(|e| { + 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.with_remote_addr(addr) - } else { - e + e = e.with_remote_addr(addr) } + + e }); if self.shared.result.set(result).is_err() { diff --git a/src/redirect.rs b/src/redirect.rs index a8706ddb..227bfc27 100644 --- a/src/redirect.rs +++ b/src/redirect.rs @@ -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. @@ -113,7 +113,7 @@ 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. diff --git a/tests/net.rs b/tests/net.rs index 635886df..a885732c 100644 --- a/tests/net.rs +++ b/tests/net.rs @@ -44,7 +44,7 @@ fn remote_addr_returns_expected_address() { } #[test] -fn remote_addr_returns_expected_address_on_error() { +fn local_and_remote_addr_returns_expected_addresses_on_error() { let server = TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); let addr = server.local_addr().unwrap(); @@ -57,6 +57,8 @@ fn remote_addr_returns_expected_address_on_error() { 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] diff --git a/tests/redirects.rs b/tests/redirects.rs index ead411fd..3e72726e 100644 --- a/tests/redirects.rs +++ b/tests/redirects.rs @@ -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! { @@ -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"); } @@ -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); From 896ad83ede78ed7b319d171b12b5fc22dd65bb89 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Sun, 22 Aug 2021 16:27:25 -0500 Subject: [PATCH 3/4] Formatting --- src/error.rs | 1 - src/handler.rs | 4 ++-- src/redirect.rs | 5 ++++- src/response.rs | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/error.rs b/src/error.rs index 0af51bf5..301685e8 100644 --- a/src/error.rs +++ b/src/error.rs @@ -380,7 +380,6 @@ impl Error { self.0.local_addr.get().cloned() } - /// Get the remote socket address of the last-used connection involved in /// this error, if known. /// diff --git a/src/handler.rs b/src/handler.rs index cd02481c..2a031427 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -212,11 +212,11 @@ impl RequestHandler { 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) + e = e.with_local_addr(addr); } if let Some(addr) = self.get_primary_addr() { - e = e.with_remote_addr(addr) + e = e.with_remote_addr(addr); } e diff --git a/src/redirect.rs b/src/redirect.rs index 227bfc27..d784c267 100644 --- a/src/redirect.rs +++ b/src/redirect.rs @@ -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(Error::with_response(ErrorKind::RequestBodyNotRewindable, &response)); + return Err(Error::with_response( + ErrorKind::RequestBodyNotRewindable, + &response, + )); } // Update the request to point to the new URI. diff --git a/src/response.rs b/src/response.rs index 4fbe83a6..af813600 100644 --- a/src/response.rs +++ b/src/response.rs @@ -274,7 +274,7 @@ impl ReadResponseExt for Response { #[cfg(feature = "text-decoding")] fn text(&mut self) -> io::Result { - 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")] @@ -430,7 +430,7 @@ impl AsyncReadResponseExt for Response { #[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")] From 897b25d94488ceec4911ee241908d3eee2b66a72 Mon Sep 17 00:00:00 2001 From: "Stephen M. Coakley" Date: Sun, 22 Aug 2021 16:31:41 -0500 Subject: [PATCH 4/4] Add local_addr to debug format --- src/error.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/error.rs b/src/error.rs index 301685e8..9a1c63ce 100644 --- a/src/error.rs +++ b/src/error.rs @@ -422,6 +422,7 @@ 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() }