From f599828c6d0ddcc90df09199567a7007d75e0a99 Mon Sep 17 00:00:00 2001 From: Ulrik Mikaelsson Date: Fri, 18 Sep 2020 22:47:11 +0200 Subject: [PATCH] Turn `Option` into `AgentState` (#149) `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. Co-authored-by: Ulrik --- src/agent.rs | 37 ++++++++++------------------- src/pool.rs | 14 +++++------ src/request.rs | 2 +- src/test/agent_test.rs | 3 +-- src/unit.rs | 54 ++++++++++++++++++------------------------ 5 files changed, 43 insertions(+), 67 deletions(-) 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
, /// Reused agent state for repeated requests from this agent. - pub(crate) state: Arc>>, + pub(crate) state: Arc>, } /// 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> { 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> PoolReturnRead { 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>>, + pub(crate) agent: Arc>, // via agent pub(crate) method: String, 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); } 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>>, + pub agent: Arc>, 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>, url: &Url) -> Option
{ +fn extract_cookies(state: &std::sync::Mutex, url: &Url) -> Option
{ 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>, _url: &Url) -> Option
{ +fn extract_cookies(_state: &std::sync::Mutex, _url: &Url) -> Option
{ 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) } } }