From 498e19943f8b69168bc07e73d6b25ab71f4a43e6 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 16 Jan 2023 08:33:56 -0800 Subject: [PATCH] Allow non-empty connection pools to be dropped (#583) This breaks a reference cycle between PoolReturner and Agent that was causing Agents (and their contained ConnectionPool) to never be dropped so long as there was any stream in the ConnectionPool. This cause sockets to leak over time, particularly when the convenience functions ureq::get(), ureq::post(), etc were used, since those functions create a new Agent each time. --- CHANGELOG.md | 5 +++++ Cargo.toml | 2 +- src/agent.rs | 4 ++++ src/pool.rs | 20 +++++++++++++------- src/response.rs | 5 +---- src/stream.rs | 4 ++-- 6 files changed, 26 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e63126a..bccf298 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +# 2.6.2 + +## Fixed + * Non-empty connection pools were never dropped (#NNN) + # 2.6.1 ## Fixed diff --git a/Cargo.toml b/Cargo.toml index 72d4256..00dc9f0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "ureq" -version = "2.6.1" +version = "2.6.2" authors = ["Martin Algesten ", "Jacob Hoffman-Andrews "] description = "Simple, safe HTTP client" license = "MIT/Apache-2.0" diff --git a/src/agent.rs b/src/agent.rs index b78158e..4ed88ce 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -238,6 +238,10 @@ impl Agent { pub fn cookie_store(&self) -> CookieStoreGuard<'_> { self.state.cookie_tin.read_lock() } + + pub(crate) fn weak_state(&self) -> std::sync::Weak { + Arc::downgrade(&self.state) + } } const DEFAULT_MAX_IDLE_CONNECTIONS: usize = 100; diff --git a/src/pool.rs b/src/pool.rs index 3eab4cf..a0668f8 100644 --- a/src/pool.rs +++ b/src/pool.rs @@ -3,6 +3,7 @@ use std::collections::{HashMap, VecDeque}; use std::io::{self, Read}; use std::sync::Mutex; +use crate::agent::AgentState; use crate::stream::Stream; use crate::{Agent, Proxy}; @@ -229,14 +230,17 @@ impl PoolKey { #[derive(Clone, Debug)] pub(crate) struct PoolReturner { - inner: Option<(Agent, PoolKey)>, + // We store a weak reference to an agent state here to avoid creating + // a reference loop, since AgentState contains a ConnectionPool, which + // contains Streams, which contain PoolReturners. + inner: Option<(std::sync::Weak, PoolKey)>, } impl PoolReturner { /// A PoolReturner that returns to the given Agent's Pool. - pub(crate) fn new(agent: Agent, pool_key: PoolKey) -> Self { + pub(crate) fn new(agent: &Agent, pool_key: PoolKey) -> Self { Self { - inner: Some((agent, pool_key)), + inner: Some((agent.weak_state(), pool_key)), } } @@ -246,8 +250,10 @@ impl PoolReturner { } pub(crate) fn return_to_pool(&self, stream: Stream) { - if let Some((agent, pool_key)) = &self.inner { - agent.state.pool.add(pool_key, stream); + if let Some((weak_state, pool_key)) = &self.inner { + if let Some(state) = weak_state.upgrade() { + state.pool.add(pool_key, stream); + } } } } @@ -433,7 +439,7 @@ mod tests { let agent = Agent::new(); let pool_key = PoolKey::new(&url, None); - let stream = NoopStream::stream(PoolReturner::new(agent.clone(), pool_key)); + let stream = NoopStream::stream(PoolReturner::new(&agent, pool_key)); let mut limited_read = LimitedRead::new(stream, std::num::NonZeroUsize::new(500).unwrap()); limited_read.read_exact(&mut out_buf).unwrap(); @@ -472,7 +478,7 @@ mod tests { let stream = Stream::new( ro, "1.1.1.1:4343".parse().unwrap(), - PoolReturner::new(agent.clone(), PoolKey::from_parts("http", "1.1.1.1", 8080)), + PoolReturner::new(&agent, PoolKey::from_parts("http", "1.1.1.1", 8080)), ); let chunked = crate::chunked::Decoder::new(stream); diff --git a/src/response.rs b/src/response.rs index 2bb667a..d5535d4 100644 --- a/src/response.rs +++ b/src/response.rs @@ -1161,10 +1161,7 @@ mod tests { let stream = Stream::new( test_stream, "1.1.1.1:4343".parse().unwrap(), - PoolReturner::new( - agent.clone(), - PoolKey::from_parts("https", "example.com", 443), - ), + PoolReturner::new(&agent, PoolKey::from_parts("https", "example.com", 443)), ); Response::do_from_stream( stream, diff --git a/src/stream.rs b/src/stream.rs index 828ce68..c444b4f 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -316,7 +316,7 @@ pub(crate) fn connect_http(unit: &Unit, hostname: &str) -> Result // let port = unit.url.port().unwrap_or(80); let pool_key = PoolKey::from_parts("http", hostname, port); - let pool_returner = PoolReturner::new(unit.agent.clone(), pool_key); + let pool_returner = PoolReturner::new(&unit.agent, pool_key); connect_host(unit, hostname, port).map(|(t, r)| Stream::new(t, r, pool_returner)) } @@ -328,7 +328,7 @@ pub(crate) fn connect_https(unit: &Unit, hostname: &str) -> Result