Fix leaks with actix_http's client (#1580)

actix-http/CHANGES.md

@@ -7,6 +7,7 @@
 * Migrate cookie handling to `cookie` crate.
 * Update `sha-1` to 0.9
 * MSRV is now 1.41.1
+* Fix client leak [#1580]
 
 ## [2.0.0-alpha.4] - 2020-05-21
 

actix-http/src/client/pool.rs

@@ -2,7 +2,7 @@ use std::cell::RefCell;
 use std::collections::VecDeque;
 use std::future::Future;
 use std::pin::Pin;
-use std::rc::Rc;
+use std::rc::{Rc, Weak};
 use std::task::{Context, Poll};
 use std::time::{Duration, Instant};
 
@@ -53,16 +53,25 @@ where
         + 'static,
 {
     pub(crate) fn new(connector: T, config: ConnectorConfig) -> Self {
-        ConnectionPool(
-            Rc::new(RefCell::new(connector)),
-            Rc::new(RefCell::new(Inner {
+        let connector_rc = Rc::new(RefCell::new(connector));
+        let inner_rc = Rc::new(RefCell::new(Inner {
             config,
             acquired: 0,
             waiters: Slab::new(),
             waiters_queue: IndexSet::new(),
             available: FxHashMap::default(),
             waker: LocalWaker::new(),
-            })),
+        }));
+
+        // start support future
+        actix_rt::spawn(ConnectorPoolSupport {
+            connector: connector_rc.clone(),
+            inner: Rc::downgrade(&inner_rc),
+        });
+
+        ConnectionPool(
+            connector_rc,
+            inner_rc,
         )
     }
 }
@@ -92,12 +101,6 @@ where
     }
 
     fn call(&mut self, req: Connect) -> Self::Future {
-        // start support future
-        actix_rt::spawn(ConnectorPoolSupport {
-            connector: self.0.clone(),
-            inner: self.1.clone(),
-        });
-
         let mut connector = self.0.clone();
         let inner = self.1.clone();
 
@@ -421,7 +424,7 @@ where
     Io: AsyncRead + AsyncWrite + Unpin + 'static,
 {
     connector: T,
-    inner: Rc<RefCell<Inner<Io>>>,
+    inner: Weak<RefCell<Inner<Io>>>,
 }
 
 impl<T, Io> Future for ConnectorPoolSupport<T, Io>
@@ -435,7 +438,8 @@ where
     fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
         let this = self.project();
 
-        let mut inner = this.inner.as_ref().borrow_mut();
+        if let Some(this_inner) = this.inner.upgrade() {
+            let mut inner = this_inner.as_ref().borrow_mut();
             inner.waker.register(cx.waker());
 
             // check waiters
@@ -458,7 +462,7 @@ where
                         if let Err(conn) = tx.send(Ok(IoConnection::new(
                             io,
                             created,
-                        Some(Acquired(key.clone(), Some(this.inner.clone()))),
+                            Some(Acquired(key.clone(), Some(this_inner.clone()))),
                         ))) {
                             let (io, created) = conn.unwrap().into_inner();
                             inner.release_conn(&key, io, created);
@@ -470,7 +474,7 @@ where
                         OpenWaitingConnection::spawn(
                             key.clone(),
                             tx,
-                        this.inner.clone(),
+                            this_inner.clone(),
                             this.connector.call(connect),
                             inner.config.clone(),
                         );
@@ -480,6 +484,9 @@ where
             }
 
             Poll::Pending
+        } else {
+            Poll::Ready(())
+        }
     }
 }
 

awc/src/request.rs

@@ -586,16 +586,16 @@ mod tests {
     use super::*;
     use crate::Client;
 
-    #[test]
-    fn test_debug() {
+    #[actix_rt::test]
+    async fn test_debug() {
         let request = Client::new().get("/").header("x-test", "111");
         let repr = format!("{:?}", request);
         assert!(repr.contains("ClientRequest"));
         assert!(repr.contains("x-test"));
     }
 
-    #[test]
-    fn test_basics() {
+    #[actix_rt::test]
+    async fn test_basics() {
         let mut req = Client::new()
             .put("/")
             .version(Version::HTTP_2)
@@ -621,8 +621,8 @@ mod tests {
         let _ = req.send_body("");
     }
 
-    #[test]
-    fn test_client_header() {
+    #[actix_rt::test]
+    async fn test_client_header() {
         let req = Client::build()
             .header(header::CONTENT_TYPE, "111")
             .finish()
@@ -639,8 +639,8 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_client_header_override() {
+    #[actix_rt::test]
+    async fn test_client_header_override() {
         let req = Client::build()
             .header(header::CONTENT_TYPE, "111")
             .finish()
@@ -658,8 +658,8 @@ mod tests {
         );
     }
 
-    #[test]
-    fn client_basic_auth() {
+    #[actix_rt::test]
+    async fn client_basic_auth() {
         let req = Client::new()
             .get("/")
             .basic_auth("username", Some("password"));
@@ -685,8 +685,8 @@ mod tests {
         );
     }
 
-    #[test]
-    fn client_bearer_auth() {
+    #[actix_rt::test]
+    async fn client_bearer_auth() {
         let req = Client::new().get("/").bearer_auth("someS3cr3tAutht0k3n");
         assert_eq!(
             req.head
@@ -699,8 +699,8 @@ mod tests {
         );
     }
 
-    #[test]
-    fn client_query() {
+    #[actix_rt::test]
+    async fn client_query() {
         let req = Client::new()
             .get("/")
             .query(&[("key1", "val1"), ("key2", "val2")])

benches/server.rs

@@ -27,15 +27,16 @@ const STR: &str = "Hello World Hello World Hello World Hello World Hello World \
 
 // benchmark sending all requests at the same time
 fn bench_async_burst(c: &mut Criterion) {
+    // We are using System here, since Runtime requires preinitialized tokio
+    // Maybe add to actix_rt docs
+    let mut rt = actix_rt::System::new("test");
+
     let srv = test::start(|| {
         App::new()
             .service(web::resource("/").route(web::to(|| HttpResponse::Ok().body(STR))))
     });
 
-    // We are using System here, since Runtime requires preinitialized tokio
-    // Maybe add to actix_rt docs
     let url = srv.url("/");
-    let mut rt = actix_rt::System::new("test");
 
     c.bench_function("get_body_async_burst", move |b| {
         b.iter_custom(|iters| {