Repository: algesten/ureq
Pull request #131 (2020-08-06 20:02:23)

Normalize timeout ErrorKind::WouldBlock to be TimedOut
`set_read_timeout` and `set_write_timeout` can cause `ErrorKind::WouldBlock` on unix-y platforms.

This PR normalize those cases to `ErrorKind::TimedOut`. This will make it simpler higher up in the stack to deal with timeouts.

This PR should land before #123 

algesten: @jsha This is what we talked about I think.

Patch:
diff --git a/src/header.rs b/src/header.rs
index 719ccb9..b788c39 100644
--- a/src/header.rs
+++ b/src/header.rs
@@ -1,7 +1,7 @@
 use crate::error::Error;
 use std::str::FromStr;
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 /// Wrapper type for a header field.
 /// https://tools.ietf.org/html/rfc7230#section-3.2
 pub struct Header {
diff --git a/src/stream.rs b/src/stream.rs
index 042821e..31a56ec 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -73,7 +73,16 @@ impl Read for DeadlineStream {
                 socket.set_write_timeout(Some(timeout))?;
             }
         }
-        self.stream.read(buf)
+        self.stream.read(buf).map_err(|e| {
+            // On unix-y platforms set_read_timeout and set_write_timeout
+            // causes ErrorKind::WouldBlock instead of ErrorKind::TimedOut.
+            // Since the socket most definitely not set_nonblocking(true),
+            // we can safely normalize WouldBlock to TimedOut
+            if e.kind() == ErrorKind::WouldBlock {
+                return io_error_timeout();
+            }
+            e
+        })
     }
 }
 
@@ -82,14 +91,15 @@ impl Read for DeadlineStream {
 fn time_until_deadline(deadline: Instant) -> IoResult<Duration> {
     let now = Instant::now();
     match deadline.checked_duration_since(now) {
-        None => Err(IoError::new(
-            ErrorKind::TimedOut,
-            "timed out reading response",
-        )),
+        None => Err(io_error_timeout()),
         Some(duration) => Ok(duration),
     }
 }
 
+fn io_error_timeout() -> IoError {
+    IoError::new(ErrorKind::TimedOut, "timed out reading response")
+}
+
 impl ::std::fmt::Debug for Stream {
     fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::result::Result<(), ::std::fmt::Error> {
         write!(
diff --git a/src/unit.rs b/src/unit.rs
index 964214c..37530e8 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -61,7 +61,7 @@ impl Unit {
 
         let query_string = combine_query(&url, &req.query, mix_queries);
 
-        let cookie_headers: Vec<_> = extract_cookies(&req.agent, &url);
+        let cookie_header: Option<Header> = extract_cookies(&req.agent, &url);
 
         let extra_headers = {
             let mut extra = vec![];
@@ -86,7 +86,7 @@ impl Unit {
         let headers: Vec<_> = req
             .headers
             .iter()
-            .chain(cookie_headers.iter())
+            .chain(cookie_header.iter())
             .chain(extra_headers.iter())
             .cloned()
             .collect();
@@ -239,51 +239,56 @@ pub(crate) fn connect(
 }
 
 #[cfg(feature = "cookie")]
-fn extract_cookies(state: &std::sync::Mutex<Option<AgentState>>, url: &Url) -> Vec<Header> {
+fn extract_cookies(state: &std::sync::Mutex<Option<AgentState>>, url: &Url) -> Option<Header> {
     let state = state.lock().unwrap();
     let is_secure = url.scheme().eq_ignore_ascii_case("https");
     let hostname = url.host_str().unwrap_or(DEFAULT_HOST).to_string();
 
-    match state.as_ref().map(|state| &state.jar) {
-        None => vec![],
-        Some(jar) => match_cookies(jar, &hostname, url.path(), is_secure),
-    }
+    state
+        .as_ref()
+        .map(|state| &state.jar)
+        .and_then(|jar| match_cookies(jar, &hostname, url.path(), is_secure))
 }
 
 #[cfg(not(feature = "cookie"))]
-fn extract_cookies(_state: &std::sync::Mutex<Option<AgentState>>, _url: &Url) -> Vec<Header> {
-    vec![]
+fn extract_cookies(_state: &std::sync::Mutex<Option<AgentState>>, _url: &Url) -> Option<Header> {
+    None
 }
 
 // TODO check so cookies can't be set for tld:s
 #[cfg(feature = "cookie")]
-fn match_cookies(jar: &CookieJar, domain: &str, path: &str, is_secure: bool) -> Vec<Header> {
-    jar.iter()
-        .filter(|c| {
-            // if there is a domain, it must be matched.
-            // if there is no domain, then ignore cookie
-            let domain_ok = c
-                .domain()
-                .map(|cdom| domain.contains(cdom))
-                .unwrap_or(false);
-            // a path must match the beginning of request path.
-            // no cookie path, we say is ok. is it?!
-            let path_ok = c
-                .path()
-                .map(|cpath| path.find(cpath).map(|pos| pos == 0).unwrap_or(false))
-                .unwrap_or(true);
-            // either the cookie isnt secure, or we're not doing a secure request.
-            let secure_ok = !c.secure().unwrap_or(false) || is_secure;
-
-            domain_ok && path_ok && secure_ok
-        })
-        .map(|c| {
-            let name = c.name().to_string();
-            let value = c.value().to_string();
-            let nameval = Cookie::new(name, value).encoded().to_string();
-            Header::new("Cookie", &nameval)
-        })
-        .collect()
+fn match_cookies(jar: &CookieJar, domain: &str, path: &str, is_secure: bool) -> Option<Header> {
+    Some(
+        jar.iter()
+            .filter(|c| {
+                // if there is a domain, it must be matched.
+                // if there is no domain, then ignore cookie
+                let domain_ok = c
+                    .domain()
+                    .map(|cdom| domain.contains(cdom))
+                    .unwrap_or(false);
+                // a path must match the beginning of request path.
+                // no cookie path, we say is ok. is it?!
+                let path_ok = c
+                    .path()
+                    .map(|cpath| path.find(cpath).map(|pos| pos == 0).unwrap_or(false))
+                    .unwrap_or(true);
+                // either the cookie isnt secure, or we're not doing a secure request.
+                let secure_ok = !c.secure().unwrap_or(false) || is_secure;
+
+                domain_ok && path_ok && secure_ok
+            })
+            .map(|c| {
+                let name = c.name().to_string();
+                let value = c.value().to_string();
+                let nameval = Cookie::new(name, value).encoded().to_string();
+                nameval
+            })
+            .collect::<Vec<_>>()
+            .join(";"),
+    )
+    .filter(|x| !x.is_empty())
+    .map(|s| Header::new("Cookie", &s))
 }
 
 /// Combine the query of the url and the query options set on the request object.
@@ -419,3 +424,39 @@ fn save_cookies(unit: &Unit, resp: &Response) {
         }
     }
 }
+
+#[cfg(test)]
+#[cfg(feature = "cookies")]
+mod tests {
+    use super::*;
+
+    ///////////////////// COOKIE TESTS //////////////////////////////
+
+    #[test]
+    fn match_cookies_returns_nothing_when_no_cookies() {
+        let jar = CookieJar::new();
+
+        let result = match_cookies(&jar, "crates.io", "/", false);
+        assert_eq!(result, None);
+    }
+
+    #[test]
+    fn match_cookies_returns_one_header() {
+        let mut jar = CookieJar::new();
+        let cookie1 = Cookie::parse("cookie1=value1; Domain=crates.io").unwrap();
+        let cookie2 = Cookie::parse("cookie2=value2; Domain=crates.io").unwrap();
+        jar.add(cookie1);
+        jar.add(cookie2);
+
+        // There's no guarantee to the order in which cookies are defined.
+        // Ensure that they're either in one order or the other.
+        let result = match_cookies(&jar, "crates.io", "/", false);
+        let order1 = "cookie1=value1;cookie2=value2";
+        let order2 = "cookie2=value2;cookie1=value1";
+
+        assert!(
+            result == Some(Header::new("Cookie", order1))
+                || result == Some(Header::new("Cookie", order2))
+        );
+    }
+}


Test patch:
diff --git a/src/test/timeout.rs b/src/test/timeout.rs
index 0ccafca..570dc4b 100644
--- a/src/test/timeout.rs
+++ b/src/test/timeout.rs
@@ -32,7 +32,6 @@ fn get_and_expect_timeout(url: String) {
 
     match resp.into_string() {
         Err(io_error) => match io_error.kind() {
-            io::ErrorKind::WouldBlock => Ok(()),
             io::ErrorKind::TimedOut => Ok(()),
             _ => Err(format!("{:?}", io_error)),
         },