Repository: algesten/ureq
Pull request #146 (2020-09-14 06:56:28)

CI: error on dead/unused code for features
By using `RUSTFLAGS="-D dead_code -D unused-variables -D unused"`, we tell
the compiler to upgrade warnings for unused code to errors. This combined
with a more elaborate test matrix means we can have the CI catch feature
flag combos that cause warnings.

algesten: @jsha This is ready to merge now. By using a matrix, it tests many combos of features and will help us catch "unused code" warnings. The number of test runs explodes a bit, there is a limit to how many we can have (256), but let's prune later if we hit the limit (the "exclude" directive can be used to remove entire branches).

Patch:
diff --git a/src/agent.rs b/src/agent.rs
index b7a43ee..b5b79da 100644
--- a/src/agent.rs
+++ b/src/agent.rs
@@ -40,13 +40,13 @@ pub struct Agent {
     /// Copied into each request of this agent.
     pub(crate) headers: Vec<Header>,
     /// Reused agent state for repeated requests from this agent.
-    pub(crate) state: Arc<Mutex<Option<AgentState>>>,
+    pub(crate) state: Arc<Mutex<AgentState>>,
 }
 
 /// Container of the state
 ///
 /// *Internal API*.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub(crate) struct AgentState {
     /// Reused connections between requests.
     pub(crate) pool: ConnectionPool,
@@ -89,7 +89,7 @@ impl Agent {
     pub fn build(&self) -> Self {
         Agent {
             headers: self.headers.clone(),
-            state: Arc::new(Mutex::new(Some(AgentState::new()))),
+            state: Arc::new(Mutex::new(AgentState::new())),
         }
     }
 
@@ -175,10 +175,8 @@ impl Agent {
     /// agent.set_max_pool_connections(200);
     /// ```
     pub fn set_max_pool_connections(&self, max_connections: usize) {
-        let mut optional_state = self.state.lock().unwrap();
-        if let Some(state) = optional_state.as_mut() {
-            state.pool.set_max_idle_connections(max_connections);
-        }
+        let mut state = self.state.lock().unwrap();
+        state.pool.set_max_idle_connections(max_connections);
     }
 
     /// Sets the maximum number of connections per host to keep in the
@@ -190,12 +188,10 @@ impl Agent {
     /// agent.set_max_pool_connections_per_host(10);
     /// ```
     pub fn set_max_pool_connections_per_host(&self, max_connections: usize) {
-        let mut optional_state = self.state.lock().unwrap();
-        if let Some(state) = optional_state.as_mut() {
-            state
-                .pool
-                .set_max_idle_connections_per_host(max_connections);
-        }
+        let mut state = self.state.lock().unwrap();
+        state
+            .pool
+            .set_max_idle_connections_per_host(max_connections);
     }
 
     /// Gets a cookie in this agent by name. Cookies are available
@@ -212,10 +208,7 @@ impl Agent {
     #[cfg(feature = "cookie")]
     pub fn cookie(&self, name: &str) -> Option<Cookie<'static>> {
         let state = self.state.lock().unwrap();
-        state
-            .as_ref()
-            .and_then(|state| state.jar.get(name))
-            .cloned()
+        state.jar.get(name).cloned()
     }
 
     /// Set a cookie in this agent.
@@ -229,12 +222,7 @@ impl Agent {
     #[cfg(feature = "cookie")]
     pub fn set_cookie(&self, cookie: Cookie<'static>) {
         let mut state = self.state.lock().unwrap();
-        match state.as_mut() {
-            None => (),
-            Some(state) => {
-                state.jar.add_original(cookie);
-            }
-        }
+        state.jar.add_original(cookie);
     }
 
     /// Make a GET request from this agent.
@@ -294,7 +282,6 @@ pub(crate) fn basic_auth(user: &str, pass: &str) -> String {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::io::Read;
     use std::thread;
 
     ///////////////////// AGENT TESTS //////////////////////////////
@@ -310,6 +297,8 @@ mod tests {
     #[test]
     #[cfg(any(feature = "tls", feature = "native-tls"))]
     fn agent_pool() {
+        use std::io::Read;
+
         let agent = crate::agent();
         let url = "https://ureq.s3.eu-central-1.amazonaws.com/sherlock.txt";
         // req 1
@@ -321,8 +310,7 @@ mod tests {
         reader.read_to_end(&mut buf).unwrap();
 
         fn poolsize(agent: &Agent) -> usize {
-            let mut lock = agent.state.lock().unwrap();
-            let state = lock.as_mut().unwrap();
+            let mut state = agent.state.lock().unwrap();
             state.pool().len()
         }
         assert_eq!(poolsize(&agent), 1);
diff --git a/src/pool.rs b/src/pool.rs
index ae2dd3f..e1a3e62 100644
--- a/src/pool.rs
+++ b/src/pool.rs
@@ -19,7 +19,7 @@ const DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST: usize = 1;
 /// Invariant: Each PoolKey exists in recycle at most once and lru at most once.
 ///
 /// *Internal API*
-#[derive(Default, Debug)]
+#[derive(Debug)]
 pub(crate) struct ConnectionPool {
     // the actual pooled connection. however only one per hostname:port.
     recycle: HashMap<PoolKey, VecDeque<Stream>>,
@@ -32,14 +32,21 @@ pub(crate) struct ConnectionPool {
     max_idle_connections_per_host: usize,
 }
 
-impl ConnectionPool {
-    pub fn new() -> Self {
-        ConnectionPool {
+impl Default for ConnectionPool {
+    fn default() -> Self {
+        Self {
             max_idle_connections: DEFAULT_MAX_IDLE_CONNECTIONS,
             max_idle_connections_per_host: DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST,
-            ..Default::default()
+            recycle: HashMap::default(),
+            lru: VecDeque::default(),
         }
     }
+}
+
+impl ConnectionPool {
+    pub fn new() -> Self {
+        Self::default()
+    }
 
     pub fn set_max_idle_connections(&mut self, max_connections: usize) {
         if self.max_idle_connections == max_connections {
@@ -374,15 +381,13 @@ impl<R: Read + Sized + Into<Stream>> PoolReturnRead<R> {
             let state = &mut unit.agent.lock().unwrap();
             // bring back stream here to either go into pool or dealloc
             let stream = reader.into();
-            if let Some(agent) = state.as_mut() {
-                if !stream.is_poolable() {
-                    // just let it deallocate
-                    return;
-                }
-                // insert back into pool
-                let key = PoolKey::new(&unit.url, &unit.proxy);
-                agent.pool().add(key, stream);
+            if !stream.is_poolable() {
+                // just let it deallocate
+                return;
             }
+            // insert back into pool
+            let key = PoolKey::new(&unit.url, &unit.proxy);
+            state.pool().add(key, stream);
         }
     }
 
diff --git a/src/request.rs b/src/request.rs
index 8a8b44e..4f1e6aa 100644
--- a/src/request.rs
+++ b/src/request.rs
@@ -29,7 +29,7 @@ use super::SerdeValue;
 /// ```
 #[derive(Clone, Default)]
 pub struct Request {
-    pub(crate) agent: Arc<Mutex<Option<AgentState>>>,
+    pub(crate) agent: Arc<Mutex<AgentState>>,
 
     // via agent
     pub(crate) method: String,
diff --git a/src/unit.rs b/src/unit.rs
index a22ddd9..cdc2b3f 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -27,9 +27,8 @@ use crate::pool::DEFAULT_HOST;
 /// It's a "unit of work". Maybe a bad name for it?
 ///
 /// *Internal API*
-#[derive(Debug)]
 pub(crate) struct Unit {
-    pub agent: Arc<Mutex<Option<AgentState>>>,
+    pub agent: Arc<Mutex<AgentState>>,
     pub url: Url,
     pub is_chunked: bool,
     pub query_string: String,
@@ -239,19 +238,16 @@ pub(crate) fn connect(
 }
 
 #[cfg(feature = "cookie")]
-fn extract_cookies(state: &std::sync::Mutex<Option<AgentState>>, url: &Url) -> Option<Header> {
+fn extract_cookies(state: &std::sync::Mutex<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();
 
-    state
-        .as_ref()
-        .map(|state| &state.jar)
-        .and_then(|jar| match_cookies(jar, &hostname, url.path(), is_secure))
+    match_cookies(&state.jar, &hostname, url.path(), is_secure)
 }
 
 #[cfg(not(feature = "cookie"))]
-fn extract_cookies(_state: &std::sync::Mutex<Option<AgentState>>, _url: &Url) -> Option<Header> {
+fn extract_cookies(_state: &std::sync::Mutex<AgentState>, _url: &Url) -> Option<Header> {
     None
 }
 
@@ -309,15 +305,13 @@ fn connect_socket(unit: &Unit, use_pooled: bool) -> Result<(Stream, bool), Error
     };
     if use_pooled {
         let state = &mut unit.agent.lock().unwrap();
-        if let Some(agent) = state.as_mut() {
-            // The connection may have been closed by the server
-            // due to idle timeout while it was sitting in the pool.
-            // Loop until we find one that is still good or run out of connections.
-            while let Some(stream) = agent.pool.try_get_connection(&unit.url, &unit.proxy) {
-                let server_closed = stream.server_closed()?;
-                if !server_closed {
-                    return Ok((stream, true));
-                }
+        // The connection may have been closed by the server
+        // due to idle timeout while it was sitting in the pool.
+        // Loop until we find one that is still good or run out of connections.
+        while let Some(stream) = state.pool.try_get_connection(&unit.url, &unit.proxy) {
+            let server_closed = stream.server_closed()?;
+            if !server_closed {
+                return Ok((stream, true));
             }
         }
     }
@@ -406,20 +400,18 @@ fn save_cookies(unit: &Unit, resp: &Response) {
 
     // only lock if we know there is something to process
     let state = &mut unit.agent.lock().unwrap();
-    if let Some(add_jar) = state.as_mut().map(|state| &mut state.jar) {
-        for raw_cookie in cookies.iter() {
-            let to_parse = if raw_cookie.to_lowercase().contains("domain=") {
-                (*raw_cookie).to_string()
-            } else {
-                let host = &unit.url.host_str().unwrap_or(DEFAULT_HOST).to_string();
-                format!("{}; Domain={}", raw_cookie, host)
-            };
-            match Cookie::parse_encoded(&to_parse[..]) {
-                Err(_) => (), // ignore unparseable cookies
-                Ok(cookie) => {
-                    let cookie = cookie.into_owned();
-                    add_jar.add(cookie)
-                }
+    for raw_cookie in cookies.iter() {
+        let to_parse = if raw_cookie.to_lowercase().contains("domain=") {
+            (*raw_cookie).to_string()
+        } else {
+            let host = &unit.url.host_str().unwrap_or(DEFAULT_HOST).to_string();
+            format!("{}; Domain={}", raw_cookie, host)
+        };
+        match Cookie::parse_encoded(&to_parse[..]) {
+            Err(_) => (), // ignore unparseable cookies
+            Ok(cookie) => {
+                let cookie = cookie.into_owned();
+                state.jar.add(cookie)
             }
         }
     }


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 94379ff..8648ccc 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -3,17 +3,46 @@ on: [push, pull_request]
 name: CI
 
 jobs:
+  doc:
+    name: Docs
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@master
+      - name: Install Rust
+        uses: actions-rs/toolchain@v1
+        with:
+          toolchain: stable
+          override: true
+      - name: Docs
+        uses: actions-rs/cargo@v1
+        with:
+          command: doc
+          # Keep in sync with Cargo.toml's [package.metadata.docs.rs]
+          args: --no-default-features --no-deps --features "tls json charset cookies socks-proxy"
   build_and_test:
-    name: Rust project
+    name: Test
     runs-on: ubuntu-latest
     strategy:
       matrix:
+        tls:
+          - ""
+          - tls
+          - native-tls
         feature:
+          - ""
+          - json
           - charset
           - cookies
           - socks-proxy
+        what:
+          - "--doc"
+          - "--tests"
+        exclude:
+          - tls: ""
+            what: "--doc"
     env:
       RUST_BACKTRACE: "1"
+      RUSTFLAGS: "-D dead_code -D unused-variables -D unused"
     steps:
       - uses: actions/checkout@master
       - name: Install Rust
@@ -25,10 +54,4 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: test
-          args: --no-default-features --features "json tls ${{ matrix.feature }}"
-      - name: Docs
-        uses: actions-rs/cargo@v1
-        with:
-          command: doc
-          # Keep in sync with Cargo.toml's [package.metadata.docs.rs]
-          args: --no-default-features --no-deps --features "tls json charset cookies socks-proxy"
+          args: ${{ matrix.what }} --no-default-features --features "${{ matrix.tls }} ${{ matrix.feature }}"
diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs
index eba1ca9..8377152 100644
--- a/src/test/agent_test.rs
+++ b/src/test/agent_test.rs
@@ -1,3 +1,5 @@
+#![allow(dead_code)]
+
 use crate::test;
 use crate::test::testserver::{read_headers, TestServer};
 use std::io::{self, Write};
@@ -78,8 +80,7 @@ fn connection_reuse() {
     resp.into_string().unwrap();
 
     {
-        let mut guard_state = agent.state.lock().unwrap();
-        let mut state = guard_state.take().unwrap();
+        let mut state = agent.state.lock().unwrap();
         assert!(state.pool().len() > 0);
     }