Repository: sagebind/isahc (rust)

Issue:
Configuration to disable Expect Header being set by default
Thanks a lot for this library!
I'm learning many Rust idioms from reading the code. 

`libcurl` sets the `Expect: 100-continue` header by default for POST requests whose body exceeds a certain size. This leads to two round-trips to the server and so worse performance. [This blog post](https://gms.tf/when-curl-sends-100-continue.html) describes the issue and a possible solution. 

I see that [curl-rust](https://github.com/alexcrichton/curl-rust/blob/65cb1c2c78321a1aedb919ee2b4225879f89f594/src/easy/handler.rs#L1350) sets the `CURLOPT_HTTPHEADER` but I can't figure out how to invoke the `http_headers` method with parameters in the shape that `libcurl` expects ie: `Expect:`

I tried setting an empty `Expect` header `.header("Expect", "")` but this does not seem to deactivate the `curl` behaviour and leads to the server rejecting the request with a `417 Expectation Failed`

I tried setting the header as `.header("Expect:", "")` but this leads to a `http::Error(InvalidHeaderName)`

Any suggestions on how to set this header to disable the `libcurl` behaviour?

Thanks in advance!




Pull request:
Allow use of Expect header to be configured
Add a configuration option for changing how POST requests behave under HTTP/1.1 and the `Expect: 100-continue` request behavior. Allow the user to change the timeout to a different duration or to disable the header entirely.

Replaces #311.

Fixes #303.

Patch:
diff --git a/src/client.rs b/src/client.rs
index d0a2706..ca57d8d 100644
--- a/src/client.rs
+++ b/src/client.rs
@@ -1063,14 +1063,20 @@ impl HttpClient {
 
         easy.signal(false)?;
 
-        request
+        let request_config = request
             .extensions()
             .get::<RequestConfig>()
-            .unwrap()
-            .set_opt(&mut easy)?;
+            .unwrap();
 
+        request_config.set_opt(&mut easy)?;
         self.inner.client_config.set_opt(&mut easy)?;
 
+        // Check if we need to disable the Expect header.
+        let disable_expect_header = request_config.expect_continue
+            .as_ref()
+            .map(|x| x.is_disabled())
+            .unwrap_or_default();
+
         // Set the HTTP method to use. Curl ties in behavior with the request
         // method, so we need to configure this carefully.
         #[allow(indirect_structural_match)]
@@ -1144,6 +1150,10 @@ impl HttpClient {
             headers.append(&header_to_curl_string(name, value, title_case))?;
         }
 
+        if disable_expect_header {
+            headers.append("Expect:")?;
+        }
+
         easy.http_headers(headers)?;
 
         Ok((easy, future))
diff --git a/src/config/mod.rs b/src/config/mod.rs
index 941dd28..50d7a03 100644
--- a/src/config/mod.rs
+++ b/src/config/mod.rs
@@ -200,6 +200,73 @@ pub trait Configurable: request::WithRequestConfig {
         })
     }
 
+    /// Configure the use of the `Expect` request header when sending request
+    /// bodies with HTTP/1.1.
+    ///
+    /// By default, when sending requests containing a body of large or unknown
+    /// length over HTTP/1.1, Isahc will send the request headers first without
+    /// the body and wait for the server to respond with a 100 (Continue) status
+    /// code, as defined by [RFC 7231, Section
+    /// 5.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-5.1.1).
+    /// This gives the opportunity for the server to reject the response without
+    /// needing to first transmit the request body over the network, if the body
+    /// contents are not necessary for the server to determine an appropriate
+    /// response.
+    ///
+    /// For servers that do not support this behavior and instead simply wait
+    /// for the request body without responding with a 100 (Continue), there is
+    /// a limited timeout before the response body is sent anyway without
+    /// confirmation. The default timeout is 1 second, but this can be
+    /// configured.
+    ///
+    /// The `Expect` behavior can also be disabled entirely.
+    ///
+    /// This configuration only takes effect when using HTTP/1.1.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use std::time::Duration;
+    /// use isahc::{
+    ///     config::ExpectContinue,
+    ///     prelude::*,
+    ///     HttpClient,
+    /// };
+    ///
+    /// // Use the default behavior (enabled).
+    /// let client = HttpClient::builder()
+    ///     .expect_continue(ExpectContinue::enabled())
+    ///     // or equivalently...
+    ///     .expect_continue(ExpectContinue::default())
+    ///     // or equivalently...
+    ///     .expect_continue(true)
+    ///     .build()?;
+    ///
+    /// // Customize the timeout if the server doesn't respond with a 100
+    /// // (Continue) status.
+    /// let client = HttpClient::builder()
+    ///     .expect_continue(ExpectContinue::timeout(Duration::from_millis(200)))
+    ///     // or equivalently...
+    ///     .expect_continue(Duration::from_millis(200))
+    ///     .build()?;
+    ///
+    /// // Disable the Expect header entirely.
+    /// let client = HttpClient::builder()
+    ///     .expect_continue(ExpectContinue::disabled())
+    ///     // or equivalently...
+    ///     .expect_continue(false)
+    ///     .build()?;
+    /// # Ok::<(), isahc::Error>(())
+    /// ```
+    fn expect_continue<T>(self, expect: T) -> Self
+    where
+        T: Into<ExpectContinue>,
+    {
+        self.with_config(move |config| {
+            config.expect_continue = Some(expect.into());
+        })
+    }
+
     /// Set one or more default HTTP authentication methods to attempt to use
     /// when authenticating with the server.
     ///
@@ -872,3 +939,87 @@ impl SetOpt for IpVersion {
         })
     }
 }
+
+/// Controls the use of the `Expect` request header when sending request bodies
+/// with HTTP/1.1.
+///
+/// By default, when sending requests containing a body of large or unknown
+/// length over HTTP/1.1, Isahc will send the request headers first without the
+/// body and wait for the server to respond with a 100 (Continue) status code,
+/// as defined by [RFC 7231, Section
+/// 5.1.1](https://datatracker.ietf.org/doc/html/rfc7231#section-5.1.1). This
+/// gives the opportunity for the server to reject the response without needing
+/// to first transmit the request body over the network, if the body contents
+/// are not necessary for the server to determine an appropriate response.
+///
+/// For servers that do not support this behavior and instead simply wait for
+/// the request body without responding with a 100 (Continue), there is a
+/// limited timeout before the response body is sent anyway without
+/// confirmation. The default timeout is 1 second, but this can be configured.
+///
+/// The `Expect` behavior can also be disabled entirely.
+///
+/// This configuration only takes effect when using HTTP/1.1.
+#[derive(Clone, Debug)]
+pub struct ExpectContinue {
+    timeout: Option<Duration>,
+}
+
+impl ExpectContinue {
+    /// Enable the use of `Expect` and wait for a 100 (Continue) response with a
+    /// default timeout before sending a request body.
+    pub const fn enabled() -> Self {
+        Self::timeout(Duration::from_secs(1))
+    }
+
+    /// Enable the use of `Expect` and wait for a 100 (Continue) response, up to
+    /// the given timeout, before sending a request body.
+    pub const fn timeout(timeout: Duration) -> Self {
+        Self {
+            timeout: Some(timeout),
+        }
+    }
+
+    /// Disable the use and handling of the `Expect` request header.
+    pub const fn disabled() -> Self {
+        Self {
+            timeout: None,
+        }
+    }
+
+    pub(crate) fn is_disabled(&self) -> bool {
+        self.timeout.is_none()
+    }
+}
+
+impl Default for ExpectContinue {
+    fn default() -> Self {
+        Self::enabled()
+    }
+}
+
+impl From<bool> for ExpectContinue {
+    fn from(value: bool) -> Self {
+        if value {
+            Self::enabled()
+        } else {
+            Self::disabled()
+        }
+    }
+}
+
+impl From<Duration> for ExpectContinue {
+    fn from(value: Duration) -> Self {
+        Self::timeout(value)
+    }
+}
+
+impl SetOpt for ExpectContinue {
+    fn set_opt<H>(&self, easy: &mut Easy2<H>) -> Result<(), curl::Error> {
+        if let Some(timeout) = self.timeout {
+            easy.expect_100_timeout(timeout)
+        } else {
+            Ok(())
+        }
+    }
+}
diff --git a/src/config/request.rs b/src/config/request.rs
index d29236e..6e6551f 100644
--- a/src/config/request.rs
+++ b/src/config/request.rs
@@ -68,6 +68,7 @@ define_request_config! {
     low_speed_timeout: Option<(u32, Duration)>,
     version_negotiation: Option<VersionNegotiation>,
     automatic_decompression: Option<bool>,
+    expect_continue: Option<ExpectContinue>,
     authentication: Option<Authentication>,
     credentials: Option<Credentials>,
     tcp_keepalive: Option<Duration>,
@@ -136,6 +137,10 @@ impl SetOpt for RequestConfig {
             }
         }
 
+        if let Some(expect_continue) = self.expect_continue.as_ref() {
+            expect_continue.set_opt(easy)?;
+        }
+
         if let Some(auth) = self.authentication.as_ref() {
             auth.set_opt(easy)?;
         }
diff --git a/src/error.rs b/src/error.rs
index 31dea15..9a1c63c 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -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.
@@ -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 {
@@ -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.
     ///
@@ -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 {
@@ -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()
     }
 }
@@ -390,6 +444,8 @@ impl From<ErrorKind> 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 5c44587..92445f6 100644
--- a/src/handler.rs
+++ b/src/handler.rs
@@ -214,6 +214,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");
         }
diff --git a/src/redirect.rs b/src/redirect.rs
index a8706dd..d784c26 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,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.
diff --git a/src/response.rs b/src/response.rs
index 4fbe83a..af81360 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -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")]
@@ -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")]


Test patch:
diff --git a/tests/expect.rs b/tests/expect.rs
new file mode 100644
index 0000000..820df70
--- /dev/null
+++ b/tests/expect.rs
@@ -0,0 +1,29 @@
+use isahc::{Body, Request, prelude::*};
+use testserver::mock;
+
+#[test]
+fn expect_header_is_sent_by_default() {
+    let m = mock!();
+
+    let body = Body::from_reader("hello world".as_bytes());
+
+    isahc::post(m.url(), body).unwrap();
+
+    m.request().expect_header("expect", "100-continue");
+}
+
+#[test]
+fn expect_header_is_not_sent_when_disabled() {
+    let m = mock!();
+
+    let body = Body::from_reader("hello world".as_bytes());
+
+    Request::post(m.url())
+        .expect_continue(false)
+        .body(body)
+        .unwrap()
+        .send()
+        .unwrap();
+
+    assert!(m.request().get_header("expect").next().is_none());
+}
diff --git a/tests/net.rs b/tests/net.rs
index 8b3a1eb..a885732 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,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() {
diff --git a/tests/redirects.rs b/tests/redirects.rs
index ead411f..3e72726 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);