From 9936cd5bcd9ef4402e90fcf5b99f894030ac750d Mon Sep 17 00:00:00 2001 From: fakeshadow <24548779@qq.com> Date: Fri, 8 Jan 2021 19:51:16 +0800 Subject: [PATCH] use a non leak pool for HttpRequestInner --- src/app_service.rs | 10 ++++----- src/config.rs | 14 +++++++++++- src/request.rs | 54 +++++++++++++++++++++++++++++++++------------- src/test.rs | 4 ---- 4 files changed, 56 insertions(+), 26 deletions(-) diff --git a/src/app_service.rs b/src/app_service.rs index 442de9362..a8d7054f3 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -13,7 +13,7 @@ use crate::config::{AppConfig, AppService}; use crate::data::{DataFactory, FnDataFactory}; use crate::error::Error; use crate::guard::Guard; -use crate::request::{HttpRequest, HttpRequestPool}; +use crate::request::HttpRequest; use crate::rmap::ResourceMap; use crate::service::{AppServiceFactory, ServiceRequest, ServiceResponse}; @@ -144,7 +144,6 @@ where rmap, config, app_data: Rc::new(app_data), - pool: HttpRequestPool::create(), }) }) } @@ -159,7 +158,6 @@ where rmap: Rc, config: AppConfig, app_data: Rc, - pool: &'static HttpRequestPool, } impl Service for AppInitService @@ -177,7 +175,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.config.pool().get() { let inner = Rc::get_mut(&mut req.inner).unwrap(); inner.path.get_mut().update(&head.uri); inner.path.reset(); @@ -192,19 +190,19 @@ where self.rmap.clone(), self.config.clone(), self.app_data.clone(), - self.pool, ) }; self.service.call(ServiceRequest::new(req)) } } +// TODO: remove the drop impl as pool is not leaked anymore. impl Drop for AppInitService where T: Service, Error = Error>, { fn drop(&mut self) { - self.pool.clear(); + self.config.pool().clear(); } } diff --git a/src/config.rs b/src/config.rs index 4ec36952a..e62c36b91 100644 --- a/src/config.rs +++ b/src/config.rs @@ -8,6 +8,7 @@ use actix_service::{boxed, IntoServiceFactory, ServiceFactory}; use crate::data::{Data, DataFactory}; use crate::error::Error; use crate::guard::Guard; +use crate::request::HttpRequestPool; use crate::resource::Resource; use crate::rmap::ResourceMap; use crate::route::Route; @@ -131,11 +132,17 @@ struct AppConfigInner { secure: bool, host: String, addr: SocketAddr, + pool: HttpRequestPool, } impl AppConfig { pub(crate) fn new(secure: bool, addr: SocketAddr, host: String) -> Self { - AppConfig(Rc::new(AppConfigInner { secure, addr, host })) + AppConfig(Rc::new(AppConfigInner { + secure, + addr, + host, + pool: HttpRequestPool::default(), + })) } /// Server host name. @@ -158,6 +165,11 @@ impl AppConfig { pub fn local_addr(&self) -> SocketAddr { self.0.addr } + + #[inline] + pub(crate) fn pool(&self) -> &HttpRequestPool { + &self.0.pool + } } impl Default for AppConfig { diff --git a/src/request.rs b/src/request.rs index 82304c1af..2ab57780e 100644 --- a/src/request.rs +++ b/src/request.rs @@ -30,7 +30,6 @@ pub(crate) struct HttpRequestInner { pub(crate) app_data: SmallVec<[Rc; 4]>, rmap: Rc, config: AppConfig, - pool: &'static HttpRequestPool, } impl HttpRequest { @@ -42,7 +41,6 @@ impl HttpRequest { rmap: Rc, config: AppConfig, app_data: Rc, - pool: &'static HttpRequestPool, ) -> HttpRequest { let mut data = SmallVec::<[Rc; 4]>::new(); data.push(app_data); @@ -55,7 +53,6 @@ impl HttpRequest { rmap, config, app_data: data, - pool, }), } } @@ -287,14 +284,16 @@ impl Drop for HttpRequest { // This relies on no Weak 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.config.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.inner.config.pool().push(req); } } } @@ -363,25 +362,50 @@ impl fmt::Debug for HttpRequest { /// Request objects are added when they are dropped (see `::drop`) and re-used /// in `::call` when there are available objects in the list. /// -/// The pool's initial capacity is 128 items. -pub(crate) struct HttpRequestPool(RefCell>>); +/// The pool's default capacity is 128 items. +pub(crate) struct HttpRequestPool { + inner: RefCell>>, + 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 { - self.0.borrow_mut().pop().map(|inner| HttpRequest { inner }) + pub(crate) fn get(&self) -> Option { + 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) { + 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() } } diff --git a/src/test.rs b/src/test.rs index a76bae6a6..c97c65f6f 100644 --- a/src/test.rs +++ b/src/test.rs @@ -30,7 +30,6 @@ pub use actix_http::test::TestBuffer; 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}; @@ -549,7 +548,6 @@ impl TestRequest { Rc::new(self.rmap), self.config.clone(), Rc::new(self.app_data), - HttpRequestPool::create(), )) } @@ -571,7 +569,6 @@ impl TestRequest { Rc::new(self.rmap), self.config.clone(), Rc::new(self.app_data), - HttpRequestPool::create(), ) } @@ -588,7 +585,6 @@ impl TestRequest { Rc::new(self.rmap), self.config.clone(), Rc::new(self.app_data), - HttpRequestPool::create(), ); (req, payload)