From aae05c5614d743cffce48526d6cf1d8c217e1a93 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 28 Oct 2020 23:55:30 -0700 Subject: [PATCH] Agent tweaks for 2.0 Replace `this` idiom with `mut self`. Move idle connections constants from pool.rs to agent.rs. Remove Agent.set and some convenience request methods (leaving get, post, and put). Move max_idle_connections setting from AgentConfig to AgentBuilder (since the builder passes these to ConnectionPool and the Agent doesn't subsequently need them). Eliminate duplicate copy of proxy in AgentState; use the one in AgentConfig. --- src/agent.rs | 72 +++++++++----------------------------------------- src/pool.rs | 27 ++++++------------- src/request.rs | 20 +++++++------- 3 files changed, 29 insertions(+), 90 deletions(-) diff --git a/src/agent.rs b/src/agent.rs index ad1ea0c..ea311bc 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -14,6 +14,8 @@ use {crate::cookies::CookieTin, cookie::Cookie, cookie_store::CookieStore, url:: pub struct AgentBuilder { headers: Vec
, config: AgentConfig, + max_idle_connections: usize, + max_idle_connections_per_host: usize, /// Cookies saved between requests. /// Invariant: All cookies must have a nonempty domain and path. #[cfg(feature = "cookies")] @@ -24,8 +26,6 @@ pub struct AgentBuilder { /// Config as built by AgentBuilder and then static for the lifetime of the Agent. #[derive(Debug, Clone)] pub(crate) struct AgentConfig { - pub max_idle_connections: usize, - pub max_idle_connections_per_host: usize, pub proxy: Option, pub timeout_connect: Option, pub timeout_read: Option, @@ -45,8 +45,6 @@ pub(crate) struct AgentConfig { /// ``` /// let mut agent = ureq::agent(); /// -/// agent.set("x-my-secret-header", "very secret"); -/// /// let auth = agent /// .post("/login") /// .call(); // blocks. @@ -84,7 +82,6 @@ pub struct Agent { pub(crate) struct AgentState { /// Reused connections between requests. pub(crate) pool: ConnectionPool, - pub(crate) proxy: Option, /// Cookies saved between requests. /// Invariant: All cookies must have a nonempty domain and path. #[cfg(feature = "cookies")] @@ -100,27 +97,6 @@ impl Agent { AgentBuilder::new().build() } - /// Set a extra header field that will be present in all following requests using the agent. - /// - /// This is useful for cases like auth, where we do a number of requests before getting - /// some credential that later must be presented in a header. - /// - /// Notice that fixed headers can also be set in the `AgentBuilder`. - /// - /// ``` - /// let mut agent = ureq::agent(); - /// - /// agent.set("X-API-Key", "foobar"); - /// agent.set("Accept", "text/plain"); - /// - /// let r = agent - /// .get("/my-page") - /// .call(); - /// ``` - pub fn set(&mut self, header: &str, value: &str) { - header::add_header(&mut self.headers, Header::new(header, value)); - } - /// Request by providing the HTTP verb such as `GET`, `POST`... /// /// ``` @@ -157,11 +133,6 @@ impl Agent { self.request("GET", path) } - /// Make a HEAD request from this agent. - pub fn head(&self, path: &str) -> Request { - self.request("HEAD", path) - } - /// Make a POST request from this agent. pub fn post(&self, path: &str) -> Request { self.request("POST", path) @@ -171,35 +142,16 @@ impl Agent { pub fn put(&self, path: &str) -> Request { self.request("PUT", path) } - - /// Make a DELETE request from this agent. - pub fn delete(&self, path: &str) -> Request { - self.request("DELETE", path) - } - - /// Make a TRACE request from this agent. - pub fn trace(&self, path: &str) -> Request { - self.request("TRACE", path) - } - - /// Make a OPTIONS request from this agent. - pub fn options(&self, path: &str) -> Request { - self.request("OPTIONS", path) - } - - /// Make a PATCH request from this agent. - pub fn patch(&self, path: &str) -> Request { - self.request("PATCH", path) - } } +const DEFAULT_MAX_IDLE_CONNECTIONS: usize = 100; +const DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST: usize = 1; + impl AgentBuilder { pub fn new() -> Self { AgentBuilder { headers: vec![], config: AgentConfig { - max_idle_connections: crate::pool::DEFAULT_MAX_IDLE_CONNECTIONS, - max_idle_connections_per_host: crate::pool::DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST, proxy: None, timeout_connect: Some(Duration::from_secs(30)), timeout_read: None, @@ -209,6 +161,8 @@ impl AgentBuilder { #[cfg(feature = "tls")] tls_config: None, }, + max_idle_connections: DEFAULT_MAX_IDLE_CONNECTIONS, + max_idle_connections_per_host: DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST, resolver: StdResolver.into(), #[cfg(feature = "cookies")] cookie_store: None, @@ -221,22 +175,20 @@ impl AgentBuilder { // not implement clone, so we have to give ownership to the newly // built Agent. pub fn build(self) -> Agent { - let config = Arc::new(self.config); Agent { headers: self.headers, + config: Arc::new(self.config), state: Arc::new(AgentState { pool: ConnectionPool::new_with_limits( - config.max_idle_connections, - config.max_idle_connections_per_host, + self.max_idle_connections, + self.max_idle_connections_per_host, ), - proxy: config.proxy.clone(), #[cfg(feature = "cookies")] cookie_tin: CookieTin::new( self.cookie_store.unwrap_or_else(|| CookieStore::default()), ), resolver: self.resolver, }), - config, } } @@ -285,7 +237,7 @@ impl AgentBuilder { /// let agent = ureq::AgentBuilder::new().max_idle_connections(200).build(); /// ``` pub fn max_idle_connections(mut self, max: usize) -> Self { - self.config.max_idle_connections = max; + self.max_idle_connections = max; self } @@ -297,7 +249,7 @@ impl AgentBuilder { /// let agent = ureq::AgentBuilder::new().max_idle_connections_per_host(200).build(); /// ``` pub fn max_idle_connections_per_host(mut self, max: usize) -> Self { - self.config.max_idle_connections_per_host = max; + self.max_idle_connections_per_host = max; self } diff --git a/src/pool.rs b/src/pool.rs index 8d9b62c..7e4b97e 100644 --- a/src/pool.rs +++ b/src/pool.rs @@ -9,9 +9,6 @@ use crate::Proxy; use url::Url; -pub const DEFAULT_MAX_IDLE_CONNECTIONS: usize = 100; -pub const DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST: usize = 1; - /// Holder of recycled connections. /// /// For each PoolKey (approximately hostname and port), there may be @@ -75,14 +72,6 @@ fn remove_last_match(list: &mut VecDeque, key: &PoolKey) -> Option Self { - Self::new_with_limits( - DEFAULT_MAX_IDLE_CONNECTIONS, - DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST, - ) - } - pub(crate) fn new_with_limits( max_idle_connections: usize, max_idle_connections_per_host: usize, @@ -231,8 +220,8 @@ fn pool_connections_limit() { // Test inserting connections with different keys into the pool, // filling and draining it. The pool should evict earlier connections // when the connection limit is reached. - let pool = ConnectionPool::new(); - let hostnames = (0..DEFAULT_MAX_IDLE_CONNECTIONS * 2).map(|i| format!("{}.example", i)); + let pool = ConnectionPool::new_with_limits(10, 1); + let hostnames = (0..pool.max_idle_connections * 2).map(|i| format!("{}.example", i)); let poolkeys = hostnames.map(|hostname| PoolKey { scheme: "https".to_string(), hostname, @@ -242,9 +231,9 @@ fn pool_connections_limit() { for key in poolkeys.clone() { pool.add(key, Stream::Cursor(std::io::Cursor::new(vec![]))); } - assert_eq!(pool.len(), DEFAULT_MAX_IDLE_CONNECTIONS); + assert_eq!(pool.len(), pool.max_idle_connections); - for key in poolkeys.skip(DEFAULT_MAX_IDLE_CONNECTIONS) { + for key in poolkeys.skip(pool.max_idle_connections) { let result = pool.remove(&key); assert!(result.is_some(), "expected key was not in pool"); } @@ -256,7 +245,7 @@ fn pool_per_host_connections_limit() { // Test inserting connections with the same key into the pool, // filling and draining it. The pool should evict earlier connections // when the per-host connection limit is reached. - let pool = ConnectionPool::new(); + let pool = ConnectionPool::new_with_limits(10, 2); let poolkey = PoolKey { scheme: "https".to_string(), hostname: "example.com".to_string(), @@ -270,9 +259,9 @@ fn pool_per_host_connections_limit() { Stream::Cursor(std::io::Cursor::new(vec![])), ); } - assert_eq!(pool.len(), DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST); + assert_eq!(pool.len(), pool.max_idle_connections_per_host); - for _ in 0..DEFAULT_MAX_IDLE_CONNECTIONS_PER_HOST { + for _ in 0..pool.max_idle_connections_per_host { let result = pool.remove(&poolkey); assert!(result.is_some(), "expected key was not in pool"); } @@ -283,7 +272,7 @@ fn pool_per_host_connections_limit() { fn pool_checks_proxy() { // Test inserting different poolkeys with same address but different proxies. // Each insertion should result in an additional entry in the pool. - let pool = ConnectionPool::new(); + let pool = ConnectionPool::new_with_limits(10, 1); let url = Url::parse("zzz:///example.com").unwrap(); pool.add( diff --git a/src/request.rs b/src/request.rs index b67ee02..b20c108 100644 --- a/src/request.rs +++ b/src/request.rs @@ -118,12 +118,11 @@ impl Request { /// } /// ``` #[cfg(feature = "json")] - pub fn send_json(self, data: SerdeValue) -> Result { - let mut this = self; - if this.header("Content-Type").is_none() { - this = this.set("Content-Type", "application/json"); + pub fn send_json(mut self, data: SerdeValue) -> Result { + if self.header("Content-Type").is_none() { + self = self.set("Content-Type", "application/json"); } - this.do_call(Payload::JSON(data)) + self.do_call(Payload::JSON(data)) } /// Send data as bytes. @@ -182,15 +181,14 @@ impl Request { /// println!("{:?}", r); /// } /// ``` - pub fn send_form(self, data: &[(&str, &str)]) -> Result { - let mut this = self; - if this.header("Content-Type").is_none() { - this = this.set("Content-Type", "application/x-www-form-urlencoded"); + pub fn send_form(mut self, data: &[(&str, &str)]) -> Result { + if self.header("Content-Type").is_none() { + self = self.set("Content-Type", "application/x-www-form-urlencoded"); } let encoded = form_urlencoded::Serializer::new(String::new()) .extend_pairs(data) .finish(); - this.do_call(Payload::Bytes(&encoded.into_bytes())) + self.do_call(Payload::Bytes(&encoded.into_bytes())) } /// Send data from a reader. @@ -423,7 +421,7 @@ impl Request { } pub(crate) fn proxy(&self) -> Option { - if let Some(proxy) = &self.agent.state.proxy { + if let Some(proxy) = &self.agent.config.proxy { Some(proxy.clone()) } else { None