Merge branch 'master' into pub-path-repr

CHANGES.md

@@ -1,6 +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

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(

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 `<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>>>);
 
 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<HttpRequest> {
-        if let Some(inner) = self.0.borrow_mut().pop() {
+        self.0.borrow_mut().pop().map(HttpRequest)
-            Some(HttpRequest(inner))
-        } else {
-            None
-        }
     }
 
+    /// Clears all allocated HttpRequest objects.
     pub(crate) fn clear(&self) {
         self.0.borrow_mut().clear()
     }

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;
+        // allow both Config and Data<Config>
-        let cfg = if let Some(cfg) = req.app_data::<PayloadConfig>() {
+        let cfg = PayloadConfig::from_req(req);
-            cfg
-        } else {
-            tmp = PayloadConfig::default();
-            &tmp
-        };
 
         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 = PayloadConfig::from_req(req);
-        let cfg = if let Some(cfg) = req.app_data::<PayloadConfig>() {
-            cfg
-        } else {
-            tmp = PayloadConfig::default();
-            &tmp
-        };
 
         // 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<T>`, in that
+    /// order, and falling back to the default payload config.
+    fn from_req(req: &HttpRequest) -> &PayloadConfig {
+        req.app_data::<PayloadConfig>()
+            .or_else(|| {
+                req.app_data::<web::Data<PayloadConfig>>()
+                    .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 {
+        DEFAULT_PAYLOAD_CONFIG.clone()
-            limit: 262_144,
-            mimetype: None,
-        }
     }
 }
 
@@ -407,8 +415,9 @@ mod tests {
     use bytes::Bytes;
 
     use super::*;
-    use crate::http::header;
+    use crate::http::{header, StatusCode};
-    use crate::test::TestRequest;
+    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")