Repository: algesten/ureq
Pull request #149 (2020-09-17 18:33:12)

Turn `Option<AgentState>` into `AgentState`
`Some(AgentState)` seem to be assumed pretty much everywhere. I could not find any test or piece of code hinting at how `None` should be interpreted, or even see how a state of `None` could even be constructed.

If the `Option` don't serve a purpose (anymore), the code can be both simpler and most likely more performant without it.

algesten: > I could not find any test or piece of code hinting at how None should be interpreted, or even see how a state of None could even be constructed.

The methods from the top of the lib:

`ureq::get("https://myserver.com/")` takes the path: `ureq::request("GET, "https://myserver.com/")`. This in turn does a `Agent::new().request(method, path)`

That `Agent::new()` is just `Default::default()` which fills in `None` in the optional.

The idea here is that since these top level calls are effectively one-shot without a retained state between calls, they will just drop any connections and not keep any cookies. The most efficient way is not instantiating an agent in the first place.

Having said that there are (at least?) two reasons we might want this change anyway.

1. It's not particularly expensive and makes for cleaner code. The new behavior is that the connection will at the end of these one-shot calls be returned to the `ConnectionPool` then dropped when the entire thing is deallocated. To retain the current "optimization", we could simply set `max_idle_connections` to `0` for these kind of "one shot" agents.
2. Cookie retention on redirects. Currently, the top level calls, maybe surprisingly, do not retain cookies when following redirects. It probably is more correct to have a fully functioning implicit `Agent` for this purpose.

@jsha Do you see any holes in my thinking?

algesten: And thanks for taking time to do this PR!
jsha: @algesten Yep, your analysis makes sense! I also think it makes sense for `ureq::get()` to use a "real" agent that's capable of recycling connections. For instance, consider the case of redirects: The current behavior is that a new connection must be established for each in a chain of redirects. But with this change, the connection can be reused so long as the redirects are on the same host.

This would also make the `ureq::get()` case exercise more of the same code paths as `Agent::get()`, which is good for ensuring that bugs don't get missed because only one code path was getting common use.
algesten: > But with this change, the connection can be reused so long as the redirects are on the same host.

@jsha Good point! That means also we should just leave `max_idle_connections` on default value.

@rawler If you remove that code formatting change from the PR, we're ready to merge this.


rawler: @algesten :+1: I have dropped the formatting-change

Out of curiosity, what's the project-ambition with regards to code formatting? Separate PR:s occasionally with "cargo fmt" applied?
jsha: > Out of curiosity, what's the project-ambition with regards to code formatting? Separate PR:s occasionally with "cargo fmt" applied?

Yep! In generally I think we should aim to keep all new code formatted on commit, and periodically fix up existing code if necessary. I had originally thought that the diff I mentioned might have been a difference between your version of rustfmt and my version (I used rustfmt integration built-in to vscode). But when I run `rustfmt` manually on the command line, it produces the same diff you had. Not sure what's going on there.

Patch:
diff --git a/src/agent.rs b/src/agent.rs
index b7a43ee..357e150 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.
@@ -321,8 +309,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 2917c69..e1a3e62 100644
--- a/src/pool.rs
+++ b/src/pool.rs
@@ -381,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/src/test/agent_test.rs b/src/test/agent_test.rs
index eba1ca9..76aa797 100644
--- a/src/test/agent_test.rs
+++ b/src/test/agent_test.rs
@@ -78,8 +78,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);
     }