Merge branch 'master' into fix/app_data

awc/src/request.rs

@@ -480,6 +480,7 @@ impl ClientRequest {
         // supported, so we cannot guess Accept-Encoding HTTP header.
         if slf.response_decompress {
             // Set Accept-Encoding with compression algorithm awc is built with.
+            #[allow(clippy::vec_init_then_push)]
             #[cfg(feature = "__compress")]
             let accept_encoding = {
                 let mut encoding = vec![];
@@ -496,7 +497,11 @@ impl ClientRequest {
                 #[cfg(feature = "compress-zstd")]
                 encoding.push("zstd");
 
-                assert!(!encoding.is_empty(), "encoding cannot be empty unless __compress feature has been explictily enabled.");
+                assert!(
+                    !encoding.is_empty(),
+                    "encoding can not be empty unless __compress feature has been explicitly enabled"
+                );
+
                 encoding.join(", ")
             };
 

src/app.rs

@@ -68,36 +68,71 @@ where
         InitError = (),
     >,
 {
-    /// Set application data. Application data could be accessed
+    /// Set application (root level) data.
-    /// by using `Data<T>` extractor where `T` is data type.
     ///
-    /// **Note**: HTTP server accepts an application factory rather than
+    /// Application data stored with `App::app_data()` method is available through the
-    /// an application instance. Http server constructs an application
+    /// [`HttpRequest::app_data`](crate::HttpRequest::app_data) method at runtime.
-    /// instance for each thread, thus application data must be constructed
+    ///
-    /// multiple times. If you want to share data between different
+    /// # [`Data<T>`]
-    /// threads, a shared object should be used, e.g. `Arc`. Internally `Data` type
+    /// Any [`Data<T>`] type added here can utilize it's extractor implementation in handlers.
-    /// uses `Arc` so data could be created outside of app factory and clones could
+    /// Types not wrapped in `Data<T>` cannot use this extractor. See [its docs](Data<T>) for more
-    /// be stored via `App::app_data()` method.
+    /// about its usage and patterns.
     ///
     /// ```
     /// use std::cell::Cell;
-    /// use actix_web::{web, App, HttpResponse, Responder};
+    /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder};
     ///
     /// struct MyData {
-    ///     counter: Cell<usize>,
+    ///     count: std::cell::Cell<usize>,
     /// }
     ///
-    /// async fn index(data: web::Data<MyData>) -> impl Responder {
+    /// async fn handler(req: HttpRequest, counter: web::Data<MyData>) -> impl Responder {
-    ///     data.counter.set(data.counter.get() + 1);
+    ///     // note this cannot use the Data<T> extractor because it was not added with it
-    ///     HttpResponse::Ok()
+    ///     let incr = *req.app_data::<usize>().unwrap();
+    ///     assert_eq!(incr, 3);
+    ///
+    ///     // update counter using other value from app data
+    ///     counter.count.set(counter.count.get() + incr);
+    ///
+    ///     HttpResponse::Ok().body(counter.count.get().to_string())
     /// }
     ///
-    /// let app = App::new()
+    /// let app = App::new().service(
-    ///     .data(MyData{ counter: Cell::new(0) })
+    ///     web::resource("/")
-    ///     .service(
+    ///         .app_data(3usize)
-    ///         web::resource("/index.html").route(
+    ///         .app_data(web::Data::new(MyData { count: Default::default() }))
-    ///             web::get().to(index)));
+    ///         .route(web::get().to(handler))
+    /// );
     /// ```
+    ///
+    /// # Shared Mutable State
+    /// [`HttpServer::new`](crate::HttpServer::new) accepts an application factory rather than an
+    /// application instance; the factory closure is called on each worker thread independently.
+    /// Therefore, if you want to share a data object between different workers, a shareable object
+    /// needs to be created first, outside the `HttpServer::new` closure and cloned into it.
+    /// [`Data<T>`] is an example of such a sharable object.
+    ///
+    /// ```ignore
+    /// let counter = web::Data::new(AppStateWithCounter {
+    ///     counter: Mutex::new(0),
+    /// });
+    ///
+    /// HttpServer::new(move || {
+    ///     // move counter object into the closure and clone for each worker
+    ///
+    ///     App::new()
+    ///         .app_data(counter.clone())
+    ///         .route("/", web::get().to(handler))
+    /// })
+    /// ```
+    pub fn app_data<U: 'static>(mut self, ext: U) -> Self {
+        self.extensions.insert(ext);
+        self
+    }
+
+    /// Add application (root) data after wrapping in `Data<T>`.
+    ///
+    /// Deprecated in favor of [`app_data`](Self::app_data).
     #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
     pub fn data<U: 'static>(self, data: U) -> Self {
         self.app_data(Data::new(data))
@@ -138,18 +173,6 @@ where
         self
     }
 
-    /// Set application level arbitrary data item.
-    ///
-    /// Application data stored with `App::app_data()` method is available
-    /// via `HttpRequest::app_data()` method at runtime.
-    ///
-    /// This method could be used for storing `Data<T>` as well, in that case
-    /// data could be accessed by using `Data<T>` extractor.
-    pub fn app_data<U: 'static>(mut self, ext: U) -> Self {
-        self.extensions.insert(ext);
-        self
-    }
-
     /// Run external configuration as part of the application building
     /// process
     ///

src/app_service.rs

@@ -144,7 +144,9 @@ where
     }
 }
 
-/// Service that takes a [`Request`] and delegates to a service that take a [`ServiceRequest`].
+/// The [`Service`] that is passed to `actix-http`'s server builder.
+///
+/// Wraps a service receiving a [`ServiceRequest`] into one receiving a [`Request`].
 pub struct AppInitService<T, B>
 where
     T: Service<ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
@@ -288,6 +290,7 @@ impl ServiceFactory<ServiceRequest> for AppRoutingFactory {
     }
 }
 
+/// The Actix Web router default entry point.
 pub struct AppRouting {
     router: Router<(HttpService, Option<Rc<Extensions>>), Guards>,
     default: HttpService,

src/config.rs

@@ -64,6 +64,8 @@ impl AppService {
         (self.config, self.services)
     }
 
+    /// Clones inner config and default service, returning new `AppService` with empty service list
+    /// marked as non-root.
     pub(crate) fn clone_config(&self) -> Self {
         AppService {
             config: self.config.clone(),
@@ -73,12 +75,12 @@ impl AppService {
         }
     }
 
-    /// Service configuration
+    /// Returns reference to configuration.
     pub fn config(&self) -> &AppConfig {
         &self.config
     }
 
-    /// Default resource
+    /// Returns default handler factory.
     pub fn default_service(&self) -> Rc<HttpNewService> {
         self.default.clone()
     }
@@ -124,7 +126,7 @@ impl AppConfig {
         AppConfig { secure, host, addr }
     }
 
-    /// Needed in actix-test crate.
+    /// Needed in actix-test crate. Semver exempt.
     #[doc(hidden)]
     pub fn __priv_test_new(secure: bool, host: String, addr: SocketAddr) -> Self {
         AppConfig::new(secure, host, addr)
@@ -200,6 +202,7 @@ impl ServiceConfig {
     /// Add shared app data item.
     ///
     /// Counterpart to [`App::data()`](crate::App::data).
+    #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
     pub fn data<U: 'static>(&mut self, data: U) -> &mut Self {
         self.app_data(Data::new(data));
         self
@@ -265,6 +268,8 @@ mod tests {
     use crate::test::{call_service, init_service, read_body, TestRequest};
     use crate::{web, App, HttpRequest, HttpResponse};
 
+    // allow deprecated `ServiceConfig::data`
+    #[allow(deprecated)]
     #[actix_rt::test]
     async fn test_data() {
         let cfg = |cfg: &mut ServiceConfig| {

src/data.rs

@@ -36,6 +36,11 @@ pub(crate) type FnDataFactory =
 /// If route data is not set for a handler, using `Data<T>` extractor would cause *Internal
 /// Server Error* response.
 ///
+// TODO: document `dyn T` functionality through converting an Arc
+// TODO: note equivalence of req.app_data<Data<T>> and Data<T> extractor
+// TODO: note that data must be inserted using Data<T> in order to extract it
+///
+/// # Examples
 /// ```
 /// use std::sync::Mutex;
 /// use actix_web::{web, App, HttpResponse, Responder};

src/resource.rs

@@ -169,41 +169,38 @@ where
         self
     }
 
-    /// Provide resource specific data. This method allows to add extractor
-    /// configuration or specific state available via `Data<T>` extractor.
-    /// Provided data is available for all routes registered for the current resource.
-    /// Resource data overrides data registered by `App::data()` method.
-    ///
-    /// ```
-    /// use actix_web::{web, App, FromRequest};
-    ///
-    /// /// extract text data from request
-    /// async fn index(body: String) -> String {
-    ///     format!("Body {}!", body)
-    /// }
-    ///
-    /// fn main() {
-    ///     let app = App::new().service(
-    ///         web::resource("/index.html")
-    ///           // limit size of the payload
-    ///           .data(String::configure(|cfg| {
-    ///                cfg.limit(4096)
-    ///           }))
-    ///           .route(
-    ///               web::get()
-    ///                  // register handler
-    ///                  .to(index)
-    ///           ));
-    /// }
-    /// ```
-    #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
-    pub fn data<U: 'static>(self, data: U) -> Self {
-        self.app_data(Data::new(data))
-    }
-
     /// Add resource data.
     ///
-    /// Data of different types from parent contexts will still be accessible.
+    /// Data of different types from parent contexts will still be accessible. Any `Data<T>` types
+    /// set here can be extracted in handlers using the `Data<T>` extractor.
+    ///
+    /// # Examples
+    /// ```
+    /// use std::cell::Cell;
+    /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder};
+    ///
+    /// struct MyData {
+    ///     count: std::cell::Cell<usize>,
+    /// }
+    ///
+    /// async fn handler(req: HttpRequest, counter: web::Data<MyData>) -> impl Responder {
+    ///     // note this cannot use the Data<T> extractor because it was not added with it
+    ///     let incr = *req.app_data::<usize>().unwrap();
+    ///     assert_eq!(incr, 3);
+    ///
+    ///     // update counter using other value from app data
+    ///     counter.count.set(counter.count.get() + incr);
+    ///
+    ///     HttpResponse::Ok().body(counter.count.get().to_string())
+    /// }
+    ///
+    /// let app = App::new().service(
+    ///     web::resource("/")
+    ///         .app_data(3usize)
+    ///         .app_data(web::Data::new(MyData { count: Default::default() }))
+    ///         .route(web::get().to(handler))
+    /// );
+    /// ```
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
         self.app_data
             .get_or_insert_with(Extensions::new)
@@ -212,6 +209,14 @@ where
         self
     }
 
+    /// Add resource data after wrapping in `Data<T>`.
+    ///
+    /// Deprecated in favor of [`app_data`](Self::app_data).
+    #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
+    pub fn data<U: 'static>(self, data: U) -> Self {
+        self.app_data(Data::new(data))
+    }
+
     /// Register a new route and add handler. This route matches all requests.
     ///
     /// ```
@@ -227,7 +232,6 @@ where
     /// This is shortcut for:
     ///
     /// ```
-    /// # extern crate actix_web;
     /// # use actix_web::*;
     /// # fn index(req: HttpRequest) -> HttpResponse { unimplemented!() }
     /// App::new().service(web::resource("/").route(web::route().to(index)));
@@ -680,7 +684,7 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::NO_CONTENT);
     }
 
-    // allow deprecated App::data
+    // allow deprecated `{App, Resource}::data`
     #[allow(deprecated)]
     #[actix_rt::test]
     async fn test_data() {
@@ -714,7 +718,7 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::OK);
     }
 
-    // allow deprecated App::data
+    // allow deprecated `{App, Resource}::data`
     #[allow(deprecated)]
     #[actix_rt::test]
     async fn test_data_default_service() {

src/scope.rs

@@ -1,28 +1,23 @@
-use std::cell::RefCell;
+use std::{cell::RefCell, fmt, future::Future, mem, rc::Rc};
-use std::fmt;
-use std::future::Future;
-use std::rc::Rc;
 
 use actix_http::Extensions;
 use actix_router::{ResourceDef, Router};
-use actix_service::boxed::{self, BoxService, BoxServiceFactory};
 use actix_service::{
-    apply, apply_fn_factory, IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt,
+    apply, apply_fn_factory,
-    Transform,
+    boxed::{self, BoxService, BoxServiceFactory},
+    IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt, Transform,
 };
 use futures_core::future::LocalBoxFuture;
 use futures_util::future::join_all;
 
-use crate::config::ServiceConfig;
+use crate::{
-use crate::data::Data;
+    config::ServiceConfig,
-use crate::dev::{AppService, HttpServiceFactory};
+    data::Data,
-use crate::error::Error;
+    dev::{AppService, HttpServiceFactory},
-use crate::guard::Guard;
+    guard::Guard,
-use crate::resource::Resource;
+    rmap::ResourceMap,
-use crate::rmap::ResourceMap;
+    service::{AppServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse},
-use crate::route::Route;
+    Error, Resource, Route,
-use crate::service::{
-    AppServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse,
 };
 
 type Guards = Vec<Box<dyn Guard>>;
@@ -71,16 +66,17 @@ pub struct Scope<T = ScopeEndpoint> {
 impl Scope {
     /// Create a new scope
     pub fn new(path: &str) -> Scope {
-        let fref = Rc::new(RefCell::new(None));
+        let factory_ref = Rc::new(RefCell::new(None));
+
         Scope {
-            endpoint: ScopeEndpoint::new(fref.clone()),
+            endpoint: ScopeEndpoint::new(Rc::clone(&factory_ref)),
             rdef: path.to_string(),
             app_data: None,
             guards: Vec::new(),
             services: Vec::new(),
             default: None,
             external: Vec::new(),
-            factory_ref: fref,
+            factory_ref,
         }
     }
 }
@@ -120,40 +116,38 @@ where
         self
     }
 
-    /// Set or override application data. Application data could be accessed
-    /// by using `Data<T>` extractor where `T` is data type.
-    ///
-    /// ```
-    /// use std::cell::Cell;
-    /// use actix_web::{web, App, HttpResponse, Responder};
-    ///
-    /// struct MyData {
-    ///     counter: Cell<usize>,
-    /// }
-    ///
-    /// async fn index(data: web::Data<MyData>) -> impl Responder {
-    ///     data.counter.set(data.counter.get() + 1);
-    ///     HttpResponse::Ok()
-    /// }
-    ///
-    /// fn main() {
-    ///     let app = App::new().service(
-    ///         web::scope("/app")
-    ///             .data(MyData{ counter: Cell::new(0) })
-    ///             .service(
-    ///                 web::resource("/index.html").route(
-    ///                     web::get().to(index)))
-    ///     );
-    /// }
-    /// ```
-    #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
-    pub fn data<U: 'static>(self, data: U) -> Self {
-        self.app_data(Data::new(data))
-    }
-
     /// Add scope data.
     ///
-    /// Data of different types from parent contexts will still be accessible.
+    /// Data of different types from parent contexts will still be accessible. Any `Data<T>` types
+    /// set here can be extracted in handlers using the `Data<T>` extractor.
+    ///
+    /// # Examples
+    /// ```
+    /// use std::cell::Cell;
+    /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder};
+    ///
+    /// struct MyData {
+    ///     count: std::cell::Cell<usize>,
+    /// }
+    ///
+    /// async fn handler(req: HttpRequest, counter: web::Data<MyData>) -> impl Responder {
+    ///     // note this cannot use the Data<T> extractor because it was not added with it
+    ///     let incr = *req.app_data::<usize>().unwrap();
+    ///     assert_eq!(incr, 3);
+    ///
+    ///     // update counter using other value from app data
+    ///     counter.count.set(counter.count.get() + incr);
+    ///
+    ///     HttpResponse::Ok().body(counter.count.get().to_string())
+    /// }
+    ///
+    /// let app = App::new().service(
+    ///     web::scope("/app")
+    ///         .app_data(3usize)
+    ///         .app_data(web::Data::new(MyData { count: Default::default() }))
+    ///         .route("/", web::get().to(handler))
+    /// );
+    /// ```
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
         self.app_data
             .get_or_insert_with(Extensions::new)
@@ -162,15 +156,20 @@ where
         self
     }
 
-    /// Run external configuration as part of the scope building
+    /// Add scope data after wrapping in `Data<T>`.
-    /// process
     ///
-    /// This function is useful for moving parts of configuration to a
+    /// Deprecated in favor of [`app_data`](Self::app_data).
-    /// different module or even library. For example,
+    #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")]
-    /// some of the resource's configuration could be moved to different module.
+    pub fn data<U: 'static>(self, data: U) -> Self {
+        self.app_data(Data::new(data))
+    }
+
+    /// Run external configuration as part of the scope building process.
+    ///
+    /// This function is useful for moving parts of configuration to a different module or library.
+    /// For example, some of the resource's configuration could be moved to different module.
     ///
     /// ```
-    /// # extern crate actix_web;
     /// use actix_web::{web, middleware, App, HttpResponse};
     ///
     /// // this function could be located in different module
@@ -191,18 +190,21 @@ where
     ///         .route("/index.html", web::get().to(|| HttpResponse::Ok()));
     /// }
     /// ```
-    pub fn configure<F>(mut self, f: F) -> Self
+    pub fn configure<F>(mut self, cfg_fn: F) -> Self
     where
         F: FnOnce(&mut ServiceConfig),
     {
         let mut cfg = ServiceConfig::new();
-        f(&mut cfg);
+        cfg_fn(&mut cfg);
+
         self.services.extend(cfg.services);
         self.external.extend(cfg.external);
 
+        // TODO: add Extensions::is_empty check and conditionally insert data
         self.app_data
             .get_or_insert_with(Extensions::new)
             .extend(cfg.app_data);
+
         self
     }
 
@@ -419,7 +421,7 @@ where
         let mut rmap = ResourceMap::new(ResourceDef::root_prefix(&self.rdef));
 
         // external resources
-        for mut rdef in std::mem::take(&mut self.external) {
+        for mut rdef in mem::take(&mut self.external) {
             rmap.add(&mut rdef, None);
         }
 
@@ -995,7 +997,7 @@ mod tests {
         );
     }
 
-    // allow deprecated App::data
+    // allow deprecated {App, Scope}::data
     #[allow(deprecated)]
     #[actix_rt::test]
     async fn test_middleware_app_data() {
@@ -1035,7 +1037,7 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::OK);
     }
 
-    // allow deprecated App::data
+    // allow deprecated `{App, Scope}::data`
     #[allow(deprecated)]
     #[actix_rt::test]
     async fn test_override_data_default_service() {

src/service.rs

@@ -17,9 +17,8 @@ use crate::{
     dev::insert_slash,
     guard::Guard,
     info::ConnectionInfo,
-    request::HttpRequest,
     rmap::ResourceMap,
-    Error, HttpResponse,
+    Error, HttpRequest, HttpResponse,
 };
 
 pub trait HttpServiceFactory {

src/web.rs

@@ -43,7 +43,6 @@ pub use crate::types::*;
 /// the exposed `Params` object:
 ///
 /// ```
-/// # extern crate actix_web;
 /// use actix_web::{web, App, HttpResponse};
 ///
 /// let app = App::new().service(

tests/test_error_propagation.rs

@@ -1,12 +1,14 @@
-use actix_utils::future::{ok, Ready};
-use actix_web::dev::{Service, ServiceRequest, ServiceResponse, Transform};
-use actix_web::test::{call_service, init_service, TestRequest};
-use actix_web::{HttpResponse, ResponseError};
-use futures_util::lock::Mutex;
-use std::future::Future;
-use std::pin::Pin;
 use std::sync::Arc;
-use std::task::{Context, Poll};
+
+use actix_utils::future::{ok, Ready};
+use actix_web::{
+    dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform},
+    get,
+    test::{call_service, init_service, TestRequest},
+    ResponseError,
+};
+use futures_core::future::LocalBoxFuture;
+use futures_util::lock::Mutex;
 
 #[derive(Debug, Clone)]
 pub struct MyError;
@@ -19,10 +21,9 @@ impl std::fmt::Display for MyError {
     }
 }
 
-#[actix_web::get("/test")]
+#[get("/test")]
 async fn test() -> Result<actix_web::HttpResponse, actix_web::error::Error> {
-    Err(MyError)?;
+    return Err(MyError.into());
-    Ok(HttpResponse::NoContent().finish())
 }
 
 #[derive(Clone)]
@@ -62,11 +63,9 @@ where
 {
     type Response = ServiceResponse<B>;
     type Error = actix_web::Error;
-    type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>>>>;
+    type Future = LocalBoxFuture<'static, Result<Self::Response, Self::Error>>;
 
-    fn poll_ready(&self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+    forward_ready!(service);
-        self.service.poll_ready(cx)
-    }
 
     fn call(&self, req: ServiceRequest) -> Self::Future {
         let lock = self.was_error.clone();