From 971ba3eee14b4191ccddd6ddae0a5c135ae789e9 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 18 Jul 2020 16:17:00 +0100 Subject: [PATCH 1/2] fix continous growth of app data in pooled requests (#1609) fixes #1606 fixes #1607 --- CHANGES.md | 4 ++++ src/app_service.rs | 3 ++- src/request.rs | 22 +++++++++++++++------- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 149ff8fcb..fca46d1c1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,10 @@ # Changes ## Unreleased - 2020-xx-xx +### Fixed +* Memory leak of app data in pooled requests. [#1609] + +[#1609]: https://github.com/actix/actix-web/pull/1609 ## 3.0.0-beta.1 - 2020-07-13 diff --git a/src/app_service.rs b/src/app_service.rs index 233cfc0d5..d41cee9fd 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -10,6 +10,7 @@ use actix_router::{Path, ResourceDef, ResourceInfo, Router, Url}; use actix_service::boxed::{self, BoxService, BoxServiceFactory}; use actix_service::{fn_service, Service, ServiceFactory}; use futures_util::future::{join_all, ok, FutureExt, LocalBoxFuture}; +use tinyvec::tiny_vec; use crate::config::{AppConfig, AppService}; use crate::data::{DataFactory, FnDataFactory}; @@ -245,7 +246,7 @@ where inner.path.reset(); inner.head = head; inner.payload = payload; - inner.app_data.push(self.data.clone()); + inner.app_data = tiny_vec![self.data.clone()]; req } else { HttpRequest::new( diff --git a/src/request.rs b/src/request.rs index 85f409016..a1b42f926 100644 --- a/src/request.rs +++ b/src/request.rs @@ -276,6 +276,7 @@ impl HttpMessage for HttpRequest { impl Drop for HttpRequest { fn drop(&mut self) { + // if possible, contribute to current worker's HttpRequest allocation pool if Rc::strong_count(&self.0) == 1 { let v = &mut self.0.pool.0.borrow_mut(); if v.len() < 128 { @@ -340,25 +341,32 @@ impl fmt::Debug for HttpRequest { } } -/// Request's objects pool +/// Slab-allocated `HttpRequest` Pool +/// +/// Since request processing may yield for asynchronous events to complete, a worker may have many +/// requests in-flight at any time. Pooling requests like this amortizes the performance and memory +/// costs of allocating and de-allocating HttpRequest objects as frequently as they otherwise would. +/// +/// 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>>); 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)) } - /// Get message from the pool + /// Re-use a previously allocated (but now completed/discarded) HttpRequest object. #[inline] pub(crate) fn get_request(&self) -> Option { - if let Some(inner) = self.0.borrow_mut().pop() { - Some(HttpRequest(inner)) - } else { - None - } + self.0.borrow_mut().pop().map(HttpRequest) } + /// Clears all allocated HttpRequest objects. pub(crate) fn clear(&self) { self.0.borrow_mut().clear() } From 43c362779dabb6f021d42e345d214aaea043adf9 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 20 Jul 2020 17:40:58 +0100 Subject: [PATCH 2/2] also try extracting payload config as Data (#1610) --- CHANGES.md | 5 ++ src/types/payload.rs | 133 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 116 insertions(+), 22 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fca46d1c1..4b6697a7f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,10 +1,15 @@ # Changes ## Unreleased - 2020-xx-xx +### Changed +* `PayloadConfig` is now also considered in `Bytes` and `String` extractors when set + using `App::data`. [#1610] + ### Fixed * Memory leak of app data in pooled requests. [#1609] [#1609]: https://github.com/actix/actix-web/pull/1609 +[#1610]: https://github.com/actix/actix-web/pull/1610 ## 3.0.0-beta.1 - 2020-07-13 diff --git a/src/types/payload.rs b/src/types/payload.rs index 0efdc2c09..653abf089 100644 --- a/src/types/payload.rs +++ b/src/types/payload.rs @@ -13,10 +13,10 @@ use futures_util::future::{err, ok, Either, FutureExt, LocalBoxFuture, Ready}; use futures_util::StreamExt; use mime::Mime; -use crate::dev; use crate::extract::FromRequest; use crate::http::header; use crate::request::HttpRequest; +use crate::{dev, web}; /// Payload extractor returns request 's payload stream. /// @@ -142,13 +142,8 @@ impl FromRequest for Bytes { #[inline] fn from_request(req: &HttpRequest, payload: &mut dev::Payload) -> Self::Future { - let tmp; - let cfg = if let Some(cfg) = req.app_data::() { - cfg - } else { - tmp = PayloadConfig::default(); - &tmp - }; + // allow both Config and Data + let cfg = PayloadConfig::from_req(req); if let Err(e) = cfg.check_mimetype(req) { return Either::Right(err(e)); @@ -197,13 +192,7 @@ impl FromRequest for String { #[inline] fn from_request(req: &HttpRequest, payload: &mut dev::Payload) -> Self::Future { - let tmp; - let cfg = if let Some(cfg) = req.app_data::() { - cfg - } else { - tmp = PayloadConfig::default(); - &tmp - }; + let cfg = PayloadConfig::from_req(req); // check content-type if let Err(e) = cfg.check_mimetype(req) { @@ -237,7 +226,12 @@ impl FromRequest for String { ) } } -/// Payload configuration for request's payload. + +/// Configuration for request's payload. +/// +/// Applies to the built-in `Bytes` and `String` extractors. Note that the Payload extractor does +/// not automatically check conformance with this configuration to allow more flexibility when +/// building extractors on top of `Payload`. #[derive(Clone)] pub struct PayloadConfig { limit: usize, @@ -284,14 +278,28 @@ impl PayloadConfig { } Ok(()) } + + /// Allow payload config extraction from app data checking both `T` and `Data`, in that + /// order, and falling back to the default payload config. + fn from_req(req: &HttpRequest) -> &PayloadConfig { + req.app_data::() + .or_else(|| { + req.app_data::>() + .map(|d| d.as_ref()) + }) + .unwrap_or_else(|| &DEFAULT_PAYLOAD_CONFIG) + } } +// Allow shared refs to default. +static DEFAULT_PAYLOAD_CONFIG: PayloadConfig = PayloadConfig { + limit: 262_144, // 2^18 bytes (~256kB) + mimetype: None, +}; + impl Default for PayloadConfig { fn default() -> Self { - PayloadConfig { - limit: 262_144, - mimetype: None, - } + DEFAULT_PAYLOAD_CONFIG.clone() } } @@ -407,8 +415,9 @@ mod tests { use bytes::Bytes; use super::*; - use crate::http::header; - use crate::test::TestRequest; + use crate::http::{header, StatusCode}; + use crate::test::{call_service, init_service, TestRequest}; + use crate::{web, App, Responder}; #[actix_rt::test] async fn test_payload_config() { @@ -428,6 +437,86 @@ mod tests { assert!(cfg.check_mimetype(&req).is_ok()); } + #[actix_rt::test] + async fn test_config_recall_locations() { + async fn bytes_handler(_: Bytes) -> impl Responder { + "payload is probably json bytes" + } + + async fn string_handler(_: String) -> impl Responder { + "payload is probably json string" + } + + let mut srv = init_service( + App::new() + .service( + web::resource("/bytes-app-data") + .app_data( + PayloadConfig::default().mimetype(mime::APPLICATION_JSON), + ) + .route(web::get().to(bytes_handler)), + ) + .service( + web::resource("/bytes-data") + .data(PayloadConfig::default().mimetype(mime::APPLICATION_JSON)) + .route(web::get().to(bytes_handler)), + ) + .service( + web::resource("/string-app-data") + .app_data( + PayloadConfig::default().mimetype(mime::APPLICATION_JSON), + ) + .route(web::get().to(string_handler)), + ) + .service( + web::resource("/string-data") + .data(PayloadConfig::default().mimetype(mime::APPLICATION_JSON)) + .route(web::get().to(string_handler)), + ), + ) + .await; + + let req = TestRequest::with_uri("/bytes-app-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/bytes-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/string-app-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/string-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/bytes-app-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/bytes-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/string-app-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/string-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + } + #[actix_rt::test] async fn test_bytes() { let (req, mut pl) = TestRequest::with_header(header::CONTENT_LENGTH, "11")