improve timeout error handling & relative uri support

Author: jayjamesjay

Cargo.lock

@@ -20,6 +20,15 @@ pub enum Stream {
 }
 
 impl Stream {
+    /// Opens a TCP connection to a remote host with a connection timeout (if specified).
+    #[deprecated(
+        since = "0.12.0",
+        note = "Stream::new(uri, connect_timeout) was replaced with Stream::connect(uri, connect_timeout)"
+    )]
+    pub fn new(uri: &Uri, connect_timeout: Option<Duration>) -> Result<Stream, Error> {
+        Stream::connect(uri, connect_timeout)
+    }
+
     /// Opens a TCP connection to a remote host with a connection timeout (if specified).
     pub fn connect(uri: &Uri, connect_timeout: Option<Duration>) -> Result<Stream, Error> {
         let host = uri.host().unwrap_or("");
@@ -168,30 +177,18 @@ where
         receiver: &Receiver<Vec<u8>>,
         deadline: Instant,
     ) -> Result<(), Error> {
-        let mut result = Ok(());
-
         execute_with_deadline(deadline, |remaining_time| {
-            let mut is_complete = false;
-
             let data_read = match receiver.recv_timeout(remaining_time) {
                 Ok(data) => data,
-                Err(e) => {
-                    if e == RecvTimeoutError::Timeout {
-                        result = Err(Error::Timeout);
-                    }
-                    return true;
-                }
+                Err(e) => match e {
+                    RecvTimeoutError::Timeout => return Err(Error::Timeout),
+                    RecvTimeoutError::Disconnected => return Ok(true),
+                },
             };
 
-            if let Err(e) = self.write_all(&data_read).map_err(|e| Error::IO(e)) {
-                result = Err(e);
-                is_complete = true;
-            }
-
-            is_complete
-        });
-
-        result
+            self.write_all(&data_read).map_err(|e| Error::IO(e))?;
+            Ok(false)
+        })
     }
 }
 
@@ -236,19 +233,27 @@ where
 /// Key information about function `func`:
 /// - is provided with information about remaining time
 /// - must ensure that its execution will not take more time than specified in `remaining_time`
-/// - needs to return `true` when the operation is complete
-pub fn execute_with_deadline<F>(deadline: Instant, mut func: F)
+/// - needs to return `Some(true)` when the operation is complete, and `Some(false)` - when operation is in progress
+pub fn execute_with_deadline<F>(deadline: Instant, mut func: F) -> Result<(), Error>
 where
-    F: FnMut(Duration) -> bool,
+    F: FnMut(Duration) -> Result<bool, Error>,
 {
     loop {
         let now = Instant::now();
         let remaining_time = deadline - now;
 
-        if deadline < now || func(remaining_time) == true {
-            break;
+        if deadline < now {
+            return Err(Error::Timeout);
+        }
+
+        match func(remaining_time) {
+            Ok(true) => break,
+            Ok(false) => continue,
+            Err(e) => return Err(e),
         }
     }
+
+    Ok(())
 }
 
 /// Reads the head of HTTP response from `reader`.
@@ -475,17 +480,18 @@ mod tests {
             let star_time = Instant::now();
             let deadline = star_time + TIMEOUT;
 
-            execute_with_deadline(deadline, |_| {
+            let timeout_err = execute_with_deadline(deadline, |_| {
                 let sleep_time = Duration::from_millis(500);
                 thread::sleep(sleep_time);
 
-                false
+                Ok(false)
             });
 
             let end_time = Instant::now();
             let total_time = end_time.duration_since(star_time).as_secs();
 
             assert_eq!(total_time, TIMEOUT.as_secs());
+            assert!(timeout_err.is_err());
         }
         {
             let star_time = Instant::now();
@@ -495,8 +501,9 @@ mod tests {
                 let sleep_time = Duration::from_secs(1);
                 thread::sleep(sleep_time);
 
-                true
-            });
+                Ok(true)
+            })
+            .unwrap();
 
             let end_time = Instant::now();
             let total_time = end_time.duration_since(star_time).as_secs();

src/stream.rs

@@ -255,23 +255,10 @@ impl<'a> Uri<'a> {
         let mut resource = self.resource().to_string();
 
         resource = match &relative_uri.get(..1) {
-            Some("#") => Uri::add_part(&resource, relative_uri, "#"),
-            Some("?") => Uri::add_part(&self.path().unwrap_or("/"), relative_uri, "?"),
-            Some("/") => Uri::add_part(&resource, relative_uri, "/"),
-            Some(_) | None => {
-                let part_idx = resource.rfind("/");
-
-                match part_idx {
-                    Some(idx) => resource[..idx].to_string() + relative_uri,
-                    None => {
-                        if resource.starts_with("/") {
-                            resource.to_string() + relative_uri
-                        } else {
-                            resource.to_string() + "/" + relative_uri
-                        }
-                    }
-                }
-            }
+            Some("#") => Uri::add_part_start(&resource, relative_uri, "#"),
+            Some("?") => Uri::add_part_start(&self.path().unwrap_or("/"), relative_uri, "?"),
+            Some("/") => Uri::add_part_start(&resource, relative_uri, "/"),
+            Some(_) | None => Uri::add_part_end(&resource, relative_uri, "/"),
         };
 
         *relative_uri = if let Some(p) = self.path {
@@ -283,15 +270,43 @@ impl<'a> Uri<'a> {
         Uri::try_from(relative_uri.as_str())
     }
 
-    /// Adds a part to a base at the position definied by a separator.
-    /// If the separator is not found, concatenates 2 strings together.
-    fn add_part(base: &str, part: &str, separator: &str) -> String {
-        let part_idx = base.find(separator);
+    /// Adds a part at the beggining of the base.
+    /// Finds the first occurance of a separator in a base and the first occurance of a separator in a part.
+    /// Joins all chars before the separator from the base, separator and all chars after the separator from the part.
+    fn add_part_start(base: &str, part: &str, separator: &str) -> String {
+        let base_idx = base.find(separator);
+        Uri::add_part(base, part, separator, base_idx)
+    }
+
+    /// Adds a part at the end of the base.
+    /// Finds the last occurance of a separator in a base and the first occurance of a separator in a part.
+    /// Joins all chars before the separator from the base, separator and all chars after the separator from the part.
+    fn add_part_end(base: &str, part: &str, separator: &str) -> String {
+        let base_idx = base.rfind(separator);
+        Uri::add_part(base, part, separator, base_idx)
+    }
+
+    /// Adds a part to the base with separator in between.
+    /// Base index defines where part should be added.
+    fn add_part(base: &str, part: &str, separator: &str, base_idx: Option<usize>) -> String {
+        let mut output = String::new();
+        let part_idx = part.find(separator);
+
+        if let Some(idx) = base_idx {
+            output += &base[..idx];
+        } else {
+            output += base;
+        }
+
+        output += separator;
 
-        match part_idx {
-            Some(idx) => base[..idx].to_string() + part,
-            None => base.to_string() + part,
+        if let Some(idx) = part_idx {
+            output += &part[idx + 1..];
+        } else {
+            output += part;
         }
+
+        output
     }
 }
 
@@ -569,7 +584,7 @@ mod tests {
         "#fragment",
         "other-path",
         "#paragraph",
-        "/foo/bar/buz",
+        "./foo/bar/buz",
         "?users#1551",
     ];
 
@@ -832,11 +847,43 @@ mod tests {
 
     #[test]
     fn uri_add_part() {
-        const BASES: [&str; 2] = ["/bar/baz?query", "/bar/baz?query#some-fragment"];
-        const RESULT: &str = "/bar/baz?query#another-fragment";
+        const BASES: [&str; 2] = ["/bar/baz/fizz?query", "/bar/baz?query#some-fragment"];
+        const RESULT: [&str; 2] = [
+            "/bar/baz/fizz?query#another-fragment",
+            "/bar/baz?query#some-fragment#another-fragment",
+        ];
+
+        for i in 0..BASES.len() {
+            assert_eq!(
+                Uri::add_part(BASES[i], "#another-fragment", "#", Some(BASES[i].len())),
+                RESULT[i]
+            );
+        }
+    }
+
+    #[test]
+    fn uri_add_part_start() {
+        const BASES: [&str; 2] = ["/bar/baz/fizz?query", "/bar/baz?query#some-fragment"];
+        const RESULT: [&str; 2] = [
+            "/bar/baz/fizz?query#another-fragment",
+            "/bar/baz?query#another-fragment",
+        ];
+
+        for i in 0..BASES.len() {
+            assert_eq!(
+                Uri::add_part_start(BASES[i], "#another-fragment", "#"),
+                RESULT[i]
+            );
+        }
+    }
+
+    #[test]
+    fn uri_add_part_end() {
+        const BASES: [&str; 2] = ["/bar/baz/fizz?query", "/bar/baz?query#some-fragment"];
+        const RESULT: [&str; 2] = ["/bar/baz/another", "/bar/another"];
 
         for i in 0..BASES.len() {
-            assert_eq!(Uri::add_part(BASES[i], "#another-fragment", "#"), RESULT);
+            assert_eq!(Uri::add_part_end(BASES[i], "./another", "/"), RESULT[i]);
         }
     }