From 46b2f7eaaf602d9e30e086b7da18c089564caf0d Mon Sep 17 00:00:00 2001 From: fakeshadow <24548779@qq.com> Date: Mon, 11 Jan 2021 06:59:44 +0800 Subject: [PATCH] use a non leak pool for HttpRequestInner (#1889) Co-authored-by: Rob Ede --- src/app_service.rs | 48 +++++++++++++++++----- src/config.rs | 12 +++--- src/request.rs | 100 +++++++++++++++++++++++++++++++++------------ src/server.rs | 36 +++++++--------- src/test.rs | 29 ++++++------- 5 files changed, 144 insertions(+), 81 deletions(-) diff --git a/src/app_service.rs b/src/app_service.rs index 3cfd84d5..c4ac0b09 100644 --- a/src/app_service.rs +++ b/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, Error = Error>, { service: T, + app_data: Rc, + app_state: Rc, +} + +// a collection of AppInitService state that shared between HttpRequests. +pub(crate) struct AppInitServiceState { rmap: Rc, config: AppConfig, - app_data: Rc, - pool: &'static HttpRequestPool, + pool: HttpRequestPool, +} + +impl AppInitServiceState { + pub(crate) fn new(rmap: Rc, config: AppConfig) -> Rc { + 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 Service for AppInitService @@ -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, Error = Error>, { fn drop(&mut self) { - self.pool.clear(); + self.app_state.pool().clear(); } } diff --git a/src/config.rs b/src/config.rs index 4ec36952..2b93ae89 100644 --- a/src/config.rs +++ b/src/config.rs @@ -125,9 +125,7 @@ impl AppService { /// Application connection config #[derive(Clone)] -pub struct AppConfig(Rc); - -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 } } diff --git a/src/request.rs b/src/request.rs index f8160ae4..437d07b6 100644 --- a/src/request.rs +++ b/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, pub(crate) payload: Payload, pub(crate) app_data: SmallVec<[Rc; 4]>, - rmap: Rc, - config: AppConfig, - pool: &'static HttpRequestPool, + app_state: Rc, } impl HttpRequest { @@ -40,10 +39,8 @@ impl HttpRequest { path: Path, head: Message, payload: Payload, - rmap: Rc, - config: AppConfig, + app_state: Rc, app_data: Rc, - pool: &'static HttpRequestPool, ) -> HttpRequest { let mut data = SmallVec::<[Rc; 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 { - 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, I: AsRef, { - 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 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 `::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 pop(&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() } } @@ -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( diff --git a/src/server.rs b/src/server.rs index 26089ccb..8bfb27b7 100644 --- a/src/server.rs +++ b/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) diff --git a/src/test.rs b/src/test.rs index a76bae6a..271ed450 100644 --- a/src/test.rs +++ b/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)