use a non leak pool for HttpRequestInner (#1889)

Co-authored-by: Rob Ede <robjtede@icloud.com>

src/app_service.rs

@@ -142,10 +142,8 @@ where
 
             Ok(AppInitService {
                 service,
-                rmap,
-                config,
                 app_data: Rc::new(app_data),
-                pool: HttpRequestPool::create(),
+                app_state: AppInitServiceState::new(rmap, config),
             })
         })
     }
@@ -157,10 +155,42 @@ where
     T: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
 {
     service: T,
+    app_data: Rc<Extensions>,
+    app_state: Rc<AppInitServiceState>,
+}
+
+// a collection of AppInitService state that shared between HttpRequests.
+pub(crate) struct AppInitServiceState {
     rmap: Rc<ResourceMap>,
     config: AppConfig,
-    app_data: Rc<Extensions>,
-    pool: &'static HttpRequestPool,
+    pool: HttpRequestPool,
+}
+
+impl AppInitServiceState {
+    pub(crate) fn new(rmap: Rc<ResourceMap>, config: AppConfig) -> Rc<Self> {
+        Rc::new(AppInitServiceState {
+            rmap,
+            config,
+            // TODO: AppConfig can be used to pass user defined HttpRequestPool
+            // capacity.
+            pool: HttpRequestPool::default(),
+        })
+    }
+
+    #[inline]
+    pub(crate) fn rmap(&self) -> &ResourceMap {
+        &*self.rmap
+    }
+
+    #[inline]
+    pub(crate) fn config(&self) -> &AppConfig {
+        &self.config
+    }
+
+    #[inline]
+    pub(crate) fn pool(&self) -> &HttpRequestPool {
+        &self.pool
+    }
 }
 
 impl<T, B> Service<Request> for AppInitService<T, B>
@@ -178,7 +208,7 @@ where
     fn call(&mut self, req: Request) -> Self::Future {
         let (head, payload) = req.into_parts();
 
-        let req = if let Some(mut req) = self.pool.get_request() {
+        let req = if let Some(mut req) = self.app_state.pool().pop() {
             let inner = Rc::get_mut(&mut req.inner).unwrap();
             inner.path.get_mut().update(&head.uri);
             inner.path.reset();
@@ -190,10 +220,8 @@ where
                 Path::new(Url::new(head.uri.clone())),
                 head,
                 payload,
-                self.rmap.clone(),
-                self.config.clone(),
+                self.app_state.clone(),
                 self.app_data.clone(),
-                self.pool,
             )
         };
         self.service.call(ServiceRequest::new(req))
@@ -205,7 +233,7 @@ where
     T: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
 {
     fn drop(&mut self) {
-        self.pool.clear();
+        self.app_state.pool().clear();
     }
 }
 

src/config.rs

@@ -125,9 +125,7 @@ impl AppService {
 
 /// Application connection config
 #[derive(Clone)]
-pub struct AppConfig(Rc<AppConfigInner>);
-
-struct AppConfigInner {
+pub struct AppConfig {
     secure: bool,
     host: String,
     addr: SocketAddr,
@@ -135,7 +133,7 @@ struct AppConfigInner {
 
 impl AppConfig {
     pub(crate) fn new(secure: bool, addr: SocketAddr, host: String) -> Self {
-        AppConfig(Rc::new(AppConfigInner { secure, addr, host }))
+        AppConfig { secure, addr, host }
     }
 
     /// Server host name.
@@ -146,17 +144,17 @@ impl AppConfig {
     ///
     /// By default host name is set to a "localhost" value.
     pub fn host(&self) -> &str {
-        &self.0.host
+        &self.host
     }
 
     /// Returns true if connection is secure(https)
     pub fn secure(&self) -> bool {
-        self.0.secure
+        self.secure
     }
 
     /// Returns the socket address of the local half of this TCP connection
     pub fn local_addr(&self) -> SocketAddr {
-        self.0.addr
+        self.addr
     }
 }
 

src/request.rs

@@ -8,6 +8,7 @@ use actix_router::{Path, Url};
 use futures_util::future::{ok, Ready};
 use smallvec::SmallVec;
 
+use crate::app_service::AppInitServiceState;
 use crate::config::AppConfig;
 use crate::error::UrlGenerationError;
 use crate::extract::FromRequest;
@@ -29,9 +30,7 @@ pub(crate) struct HttpRequestInner {
     pub(crate) path: Path<Url>,
     pub(crate) payload: Payload,
     pub(crate) app_data: SmallVec<[Rc<Extensions>; 4]>,
-    rmap: Rc<ResourceMap>,
-    config: AppConfig,
-    pool: &'static HttpRequestPool,
+    app_state: Rc<AppInitServiceState>,
 }
 
 impl HttpRequest {
@@ -40,10 +39,8 @@ impl HttpRequest {
         path: Path<Url>,
         head: Message<RequestHead>,
         payload: Payload,
-        rmap: Rc<ResourceMap>,
-        config: AppConfig,
+        app_state: Rc<AppInitServiceState>,
         app_data: Rc<Extensions>,
-        pool: &'static HttpRequestPool,
     ) -> HttpRequest {
         let mut data = SmallVec::<[Rc<Extensions>; 4]>::new();
         data.push(app_data);
@@ -53,10 +50,8 @@ impl HttpRequest {
                 head,
                 path,
                 payload,
-                rmap,
-                config,
+                app_state,
                 app_data: data,
-                pool,
             }),
         }
     }
@@ -142,7 +137,7 @@ impl HttpRequest {
     /// Returns a None when no resource is fully matched, including default services.
     #[inline]
     pub fn match_pattern(&self) -> Option<String> {
-        self.inner.rmap.match_pattern(self.path())
+        self.resource_map().match_pattern(self.path())
     }
 
     /// The resource name that matched the path. Useful for logging and metrics.
@@ -150,7 +145,7 @@ impl HttpRequest {
     /// Returns a None when no resource is fully matched, including default services.
     #[inline]
     pub fn match_name(&self) -> Option<&str> {
-        self.inner.rmap.match_name(self.path())
+        self.resource_map().match_name(self.path())
     }
 
     /// Request extensions
@@ -192,7 +187,7 @@ impl HttpRequest {
         U: IntoIterator<Item = I>,
         I: AsRef<str>,
     {
-        self.inner.rmap.url_for(&self, name, elements)
+        self.resource_map().url_for(&self, name, elements)
     }
 
     /// Generate url for named resource
@@ -207,7 +202,7 @@ impl HttpRequest {
     #[inline]
     /// Get a reference to a `ResourceMap` of current application.
     pub fn resource_map(&self) -> &ResourceMap {
-        &self.inner.rmap
+        &self.app_state().rmap()
     }
 
     /// Peer socket address
@@ -227,13 +222,13 @@ impl HttpRequest {
     /// borrowed.
     #[inline]
     pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> {
-        ConnectionInfo::get(self.head(), &*self.app_config())
+        ConnectionInfo::get(self.head(), self.app_config())
     }
 
     /// App config
     #[inline]
     pub fn app_config(&self) -> &AppConfig {
-        &self.inner.config
+        self.app_state().config()
     }
 
     /// Get an application data object stored with `App::data` or `App::app_data`
@@ -253,6 +248,11 @@ impl HttpRequest {
 
         None
     }
+
+    #[inline]
+    fn app_state(&self) -> &AppInitServiceState {
+        &*self.inner.app_state
+    }
 }
 
 impl HttpMessage for HttpRequest {
@@ -288,14 +288,16 @@ impl Drop for HttpRequest {
 
         // This relies on no Weak<HttpRequestInner> exists anywhere.(There is none)
         if let Some(inner) = Rc::get_mut(&mut self.inner) {
-            let v = &mut inner.pool.0.borrow_mut();
-            if v.len() < 128 {
+            if inner.app_state.pool().is_available() {
                 // clear additional app_data and keep the root one for reuse.
                 inner.app_data.truncate(1);
                 // inner is borrowed mut here. get head's Extension mutably
                 // to reduce borrow check
                 inner.head.extensions.get_mut().clear();
-                v.push(self.inner.clone());
+
+                // a re-borrow of pool is necessary here.
+                let req = self.inner.clone();
+                self.app_state().pool().push(req);
             }
         }
     }
@@ -364,25 +366,50 @@ impl fmt::Debug for HttpRequest {
 /// Request objects are added when they are dropped (see `<HttpRequest as Drop>::drop`) and re-used
 /// in `<AppInitService as Service>::call` when there are available objects in the list.
 ///
-/// The pool's initial capacity is 128 items.
-pub(crate) struct HttpRequestPool(RefCell<Vec<Rc<HttpRequestInner>>>);
+/// The pool's default capacity is 128 items.
+pub(crate) struct HttpRequestPool {
+    inner: RefCell<Vec<Rc<HttpRequestInner>>>,
+    cap: usize,
+}
+
+impl Default for HttpRequestPool {
+    fn default() -> Self {
+        Self::with_capacity(128)
+    }
+}
 
 impl HttpRequestPool {
-    /// Allocates a slab of memory for pool use.
-    pub(crate) fn create() -> &'static HttpRequestPool {
-        let pool = HttpRequestPool(RefCell::new(Vec::with_capacity(128)));
-        Box::leak(Box::new(pool))
+    pub(crate) fn with_capacity(cap: usize) -> Self {
+        HttpRequestPool {
+            inner: RefCell::new(Vec::with_capacity(cap)),
+            cap,
+        }
     }
 
     /// Re-use a previously allocated (but now completed/discarded) HttpRequest object.
     #[inline]
-    pub(crate) fn get_request(&self) -> Option<HttpRequest> {
-        self.0.borrow_mut().pop().map(|inner| HttpRequest { inner })
+    pub(crate) fn pop(&self) -> Option<HttpRequest> {
+        self.inner
+            .borrow_mut()
+            .pop()
+            .map(|inner| HttpRequest { inner })
+    }
+
+    /// Check if the pool still has capacity for request storage.
+    #[inline]
+    pub(crate) fn is_available(&self) -> bool {
+        self.inner.borrow_mut().len() < self.cap
+    }
+
+    /// Push a request to pool.
+    #[inline]
+    pub(crate) fn push(&self, req: Rc<HttpRequestInner>) {
+        self.inner.borrow_mut().push(req);
     }
 
     /// Clears all allocated HttpRequest objects.
     pub(crate) fn clear(&self) {
-        self.0.borrow_mut().clear()
+        self.inner.borrow_mut().clear()
     }
 }
 
@@ -528,6 +555,25 @@ mod tests {
         );
     }
 
+    #[actix_rt::test]
+    async fn test_drop_http_request_pool() {
+        let mut srv = init_service(App::new().service(web::resource("/").to(
+            |req: HttpRequest| {
+                HttpResponse::Ok()
+                    .set_header("pool_cap", req.app_state().pool().cap)
+                    .finish()
+            },
+        )))
+        .await;
+
+        let req = TestRequest::default().to_request();
+        let resp = call_service(&mut srv, req).await;
+
+        drop(srv);
+
+        assert_eq!(resp.headers().get("pool_cap").unwrap(), "128");
+    }
+
     #[actix_rt::test]
     async fn test_data() {
         let mut srv = init_service(App::new().app_data(10usize).service(

src/server.rs

@@ -283,11 +283,7 @@ where
             lst,
             move || {
                 let c = cfg.lock().unwrap();
-                let cfg = AppConfig::new(
-                    false,
-                    addr,
-                    c.host.clone().unwrap_or_else(|| format!("{}", addr)),
-                );
+                let host = c.host.clone().unwrap_or_else(|| format!("{}", addr));
 
                 let svc = HttpService::build()
                     .keep_alive(c.keep_alive)
@@ -302,8 +298,10 @@ where
                     svc
                 };
 
-                svc.finish(map_config(factory(), move |_| cfg.clone()))
-                    .tcp()
+                svc.finish(map_config(factory(), move |_| {
+                    AppConfig::new(false, addr, host.clone())
+                }))
+                .tcp()
             },
         )?;
         Ok(self)
@@ -342,11 +340,7 @@ where
             lst,
             move || {
                 let c = cfg.lock().unwrap();
-                let cfg = AppConfig::new(
-                    true,
-                    addr,
-                    c.host.clone().unwrap_or_else(|| format!("{}", addr)),
-                );
+                let host = c.host.clone().unwrap_or_else(|| format!("{}", addr));
 
                 let svc = HttpService::build()
                     .keep_alive(c.keep_alive)
@@ -361,8 +355,10 @@ where
                     svc
                 };
 
-                svc.finish(map_config(factory(), move |_| cfg.clone()))
-                    .openssl(acceptor.clone())
+                svc.finish(map_config(factory(), move |_| {
+                    AppConfig::new(true, addr, host.clone())
+                }))
+                .openssl(acceptor.clone())
             },
         )?;
         Ok(self)
@@ -401,11 +397,7 @@ where
             lst,
             move || {
                 let c = cfg.lock().unwrap();
-                let cfg = AppConfig::new(
-                    true,
-                    addr,
-                    c.host.clone().unwrap_or_else(|| format!("{}", addr)),
-                );
+                let host = c.host.clone().unwrap_or_else(|| format!("{}", addr));
 
                 let svc = HttpService::build()
                     .keep_alive(c.keep_alive)
@@ -420,8 +412,10 @@ where
                     svc
                 };
 
-                svc.finish(map_config(factory(), move |_| cfg.clone()))
-                    .rustls(config.clone())
+                svc.finish(map_config(factory(), move |_| {
+                    AppConfig::new(true, addr, host.clone())
+                }))
+                .rustls(config.clone())
             },
         )?;
         Ok(self)

src/test.rs

@@ -27,10 +27,10 @@ use socket2::{Domain, Protocol, Socket, Type};
 
 pub use actix_http::test::TestBuffer;
 
+use crate::app_service::AppInitServiceState;
 use crate::config::AppConfig;
 use crate::data::Data;
 use crate::dev::{Body, MessageBody, Payload, Server};
-use crate::request::HttpRequestPool;
 use crate::rmap::ResourceMap;
 use crate::service::{ServiceRequest, ServiceResponse};
 use crate::{Error, HttpRequest, HttpResponse};
@@ -542,14 +542,15 @@ impl TestRequest {
         head.peer_addr = self.peer_addr;
         self.path.get_mut().update(&head.uri);
 
+        let app_state =
+            AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
+
         ServiceRequest::new(HttpRequest::new(
             self.path,
             head,
             payload,
-            Rc::new(self.rmap),
-            self.config.clone(),
+            app_state,
             Rc::new(self.app_data),
-            HttpRequestPool::create(),
         ))
     }
 
@@ -564,15 +565,10 @@ impl TestRequest {
         head.peer_addr = self.peer_addr;
         self.path.get_mut().update(&head.uri);
 
-        HttpRequest::new(
-            self.path,
-            head,
-            payload,
-            Rc::new(self.rmap),
-            self.config.clone(),
-            Rc::new(self.app_data),
-            HttpRequestPool::create(),
-        )
+        let app_state =
+            AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
+
+        HttpRequest::new(self.path, head, payload, app_state, Rc::new(self.app_data))
     }
 
     /// Complete request creation and generate `HttpRequest` and `Payload` instances
@@ -581,14 +577,15 @@ impl TestRequest {
         head.peer_addr = self.peer_addr;
         self.path.get_mut().update(&head.uri);
 
+        let app_state =
+            AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
+
         let req = HttpRequest::new(
             self.path,
             head,
             Payload::None,
-            Rc::new(self.rmap),
-            self.config.clone(),
+            app_state,
             Rc::new(self.app_data),
-            HttpRequestPool::create(),
         );
 
         (req, payload)