Remove extensions from head (#2487)

CHANGES.md

@@ -8,6 +8,7 @@
 * `HttpResponse::map_into_{left,right}_body` and `HttpResponse::map_into_boxed_body`. [#2468]
 * `ServiceResponse::map_into_{left,right}_body` and `HttpResponse::map_into_boxed_body`. [#2468]
 * Connection data set through the `HttpServer::on_connect` callback is now accessible only from the new `HttpRequest::conn_data()` and `ServiceRequest::conn_data()` methods. [#2491]
+* `HttpRequest::{req_data,req_data_mut}`. [#2487]
 
 ### Changed
 * Rename `Accept::{mime_precedence => ranked}`. [#2480]
@@ -16,18 +17,23 @@
 * `HttpRequest::url_for` no longer constructs URLs with query or fragment components. [#2430]
 * Remove `B` (body) type parameter on `App`. [#2493]
 * Add `B` (body) type parameter on `Scope`. [#2492]
+* Request-local data container is no longer part of a `RequestHead`. Instead it is a distinct part of a `Request`. [#2487]
 
 ### Fixed
 * Accept wildcard `*` items in `AcceptLanguage`. [#2480]
 * Re-exports `dev::{BodySize, MessageBody, SizedStream}`. They are exposed through the `body` module. [#2468]
 * Typed headers containing lists that require one or more items now enforce this minimum. [#2482]
 
+### Removed
+* `ConnectionInfo::get`. [#2487]
+
 [#2430]: https://github.com/actix/actix-web/pull/2430
 [#2468]: https://github.com/actix/actix-web/pull/2468
 [#2480]: https://github.com/actix/actix-web/pull/2480
 [#2482]: https://github.com/actix/actix-web/pull/2482
 [#2484]: https://github.com/actix/actix-web/pull/2484
 [#2485]: https://github.com/actix/actix-web/pull/2485
+[#2487]: https://github.com/actix/actix-web/pull/2487
 [#2491]: https://github.com/actix/actix-web/pull/2491
 [#2492]: https://github.com/actix/actix-web/pull/2492
 [#2493]: https://github.com/actix/actix-web/pull/2493

actix-http/CHANGES.md

@@ -16,6 +16,8 @@
 * `impl Display` for `header::Quality`. [#2486]
 * Connection data set through the `on_connect_ext` callbacks is now accessible only from the new `Request::conn_data()` method. [#2491]
 * `Request::take_conn_data()`. [#2491]
+* `Request::take_req_data()`. [#2487]
+* `impl Clone` for `RequestHead`. [#2487]
 
 ### Changed
 * Rename `body::BoxBody::{from_body => new}`. [#2468]
@@ -40,6 +42,7 @@
 [#2468]: https://github.com/actix/actix-web/pull/2468
 [#1920]: https://github.com/actix/actix-web/pull/1920
 [#2486]: https://github.com/actix/actix-web/pull/2486
+[#2487]: https://github.com/actix/actix-web/pull/2487
 [#2488]: https://github.com/actix/actix-web/pull/2488
 [#2491]: https://github.com/actix/actix-web/pull/2491
 

actix-http/examples/hello-world.rs

@@ -1,8 +1,9 @@
 use std::{convert::Infallible, io};
 
-use actix_http::{HttpService, Response, StatusCode};
+use actix_http::{
+    header::HeaderValue, HttpMessage, HttpService, Request, Response, StatusCode,
+};
 use actix_server::Server;
-use http::header::HeaderValue;
 
 #[actix_rt::main]
 async fn main() -> io::Result<()> {
@@ -13,12 +14,21 @@ async fn main() -> io::Result<()> {
             HttpService::build()
                 .client_timeout(1000)
                 .client_disconnect(1000)
-                .finish(|req| async move {
+                .on_connect_ext(|_, ext| {
+                    ext.insert(42u32);
+                })
+                .finish(|req: Request| async move {
                     log::info!("{:?}", req);
 
                     let mut res = Response::build(StatusCode::OK);
                     res.insert_header(("x-head", HeaderValue::from_static("dummy value!")));
 
+                    let forty_two = req.extensions().get::<u32>().unwrap().to_string();
+                    res.insert_header((
+                        "x-forty-two",
+                        HeaderValue::from_str(&forty_two).unwrap(),
+                    ));
+
                     Ok::<_, Infallible>(res.body("Hello world!"))
                 })
                 .tcp()

actix-http/src/extensions.rs

@@ -19,7 +19,7 @@ impl Extensions {
     #[inline]
     pub fn new() -> Extensions {
         Extensions {
-            map: AHashMap::default(),
+            map: AHashMap::new(),
         }
     }
 

actix-http/src/message.rs

@@ -44,13 +44,12 @@ pub trait Head: Default + 'static {
         F: FnOnce(&MessagePool<Self>) -> R;
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct RequestHead {
     pub method: Method,
     pub uri: Uri,
     pub version: Version,
     pub headers: HeaderMap,
-    pub extensions: RefCell<Extensions>,
     pub peer_addr: Option<net::SocketAddr>,
     flags: Flags,
 }
@@ -62,7 +61,6 @@ impl Default for RequestHead {
             uri: Uri::default(),
             version: Version::HTTP_11,
             headers: HeaderMap::with_capacity(16),
-            extensions: RefCell::new(Extensions::new()),
             peer_addr: None,
             flags: Flags::empty(),
         }
@@ -73,7 +71,6 @@ impl Head for RequestHead {
     fn clear(&mut self) {
         self.flags = Flags::empty();
         self.headers.clear();
-        self.extensions.get_mut().clear();
     }
 
     fn with_pool<F, R>(f: F) -> R
@@ -85,18 +82,6 @@ impl Head for RequestHead {
 }
 
 impl RequestHead {
-    /// Message extensions
-    #[inline]
-    pub fn extensions(&self) -> Ref<'_, Extensions> {
-        self.extensions.borrow()
-    }
-
-    /// Mutable reference to a the message's extensions
-    #[inline]
-    pub fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.extensions.borrow_mut()
-    }
-
     /// Read the message headers.
     pub fn headers(&self) -> &HeaderMap {
         &self.headers

actix-http/src/request.rs

@@ -1,8 +1,8 @@
 //! HTTP requests.
 
 use std::{
-    cell::{Ref, RefMut},
+    cell::{Ref, RefCell, RefMut},
-    fmt, net,
+    fmt, mem, net,
     rc::Rc,
     str,
 };
@@ -22,6 +22,7 @@ pub struct Request<P = PayloadStream> {
     pub(crate) payload: Payload<P>,
     pub(crate) head: Message<RequestHead>,
     pub(crate) conn_data: Option<Rc<Extensions>>,
+    pub(crate) req_data: RefCell<Extensions>,
 }
 
 impl<P> HttpMessage for Request<P> {
@@ -33,19 +34,19 @@ impl<P> HttpMessage for Request<P> {
     }
 
     fn take_payload(&mut self) -> Payload<P> {
-        std::mem::replace(&mut self.payload, Payload::None)
+        mem::replace(&mut self.payload, Payload::None)
     }
 
     /// Request extensions
     #[inline]
     fn extensions(&self) -> Ref<'_, Extensions> {
-        self.head.extensions()
+        self.req_data.borrow()
     }
 
     /// Mutable reference to a the request's extensions
     #[inline]
     fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.head.extensions_mut()
+        self.req_data.borrow_mut()
     }
 }
 
@@ -54,6 +55,7 @@ impl From<Message<RequestHead>> for Request<PayloadStream> {
         Request {
             head,
             payload: Payload::None,
+            req_data: RefCell::new(Extensions::default()),
             conn_data: None,
         }
     }
@@ -65,6 +67,7 @@ impl Request<PayloadStream> {
         Request {
             head: Message::new(),
             payload: Payload::None,
+            req_data: RefCell::new(Extensions::default()),
             conn_data: None,
         }
     }
@@ -76,6 +79,7 @@ impl<P> Request<P> {
         Request {
             payload,
             head: Message::new(),
+            req_data: RefCell::new(Extensions::default()),
             conn_data: None,
         }
     }
@@ -88,6 +92,7 @@ impl<P> Request<P> {
             Request {
                 payload,
                 head: self.head,
+                req_data: self.req_data,
                 conn_data: self.conn_data,
             },
             pl,
@@ -101,7 +106,7 @@ impl<P> Request<P> {
 
     /// Get request's payload
     pub fn take_payload(&mut self) -> Payload<P> {
-        std::mem::replace(&mut self.payload, Payload::None)
+        mem::replace(&mut self.payload, Payload::None)
     }
 
     /// Split request into request head and payload
@@ -124,7 +129,7 @@ impl<P> Request<P> {
 
     /// Mutable reference to the message's headers.
     pub fn headers_mut(&mut self) -> &mut HeaderMap {
-        &mut self.head_mut().headers
+        &mut self.head.headers
     }
 
     /// Request's uri.
@@ -136,7 +141,7 @@ impl<P> Request<P> {
     /// Mutable reference to the request's uri.
     #[inline]
     pub fn uri_mut(&mut self) -> &mut Uri {
-        &mut self.head_mut().uri
+        &mut self.head.uri
     }
 
     /// Read the Request method.
@@ -198,6 +203,11 @@ impl<P> Request<P> {
     pub fn take_conn_data(&mut self) -> Option<Rc<Extensions>> {
         self.conn_data.take()
     }
+
+    /// Returns the request data container, leaving an empty one in it's place.
+    pub fn take_req_data(&mut self) -> Extensions {
+        mem::take(&mut self.req_data.get_mut())
+    }
 }
 
 impl<P> fmt::Debug for Request<P> {

src/app_service.rs

@@ -198,6 +198,7 @@ where
     actix_service::forward_ready!(service);
 
     fn call(&self, mut req: Request) -> Self::Future {
+        let req_data = Rc::new(RefCell::new(req.take_req_data()));
         let conn_data = req.take_conn_data();
         let (head, payload) = req.into_parts();
 
@@ -207,6 +208,7 @@ where
             inner.path.reset();
             inner.head = head;
             inner.conn_data = conn_data;
+            inner.req_data = req_data;
             req
         } else {
             HttpRequest::new(
@@ -215,6 +217,7 @@ where
                 self.app_state.clone(),
                 self.app_data.clone(),
                 conn_data,
+                req_data,
             )
         };
         self.service.call(ServiceRequest::new(req, payload))

src/info.rs

@@ -1,4 +1,4 @@
-use std::{cell::Ref, convert::Infallible, net::SocketAddr};
+use std::{convert::Infallible, net::SocketAddr};
 
 use actix_utils::future::{err, ok, Ready};
 use derive_more::{Display, Error};
@@ -72,15 +72,7 @@ pub struct ConnectionInfo {
 }
 
 impl ConnectionInfo {
-    /// Create *ConnectionInfo* instance for a request.
+    pub(crate) fn new(req: &RequestHead, cfg: &AppConfig) -> ConnectionInfo {
-    pub fn get<'a>(req: &'a RequestHead, cfg: &AppConfig) -> Ref<'a, Self> {
-        if !req.extensions().contains::<ConnectionInfo>() {
-            req.extensions_mut().insert(ConnectionInfo::new(req, cfg));
-        }
-        Ref::map(req.extensions(), |e| e.get().unwrap())
-    }
-
-    fn new(req: &RequestHead, cfg: &AppConfig) -> ConnectionInfo {
         let mut host = None;
         let mut scheme = None;
         let mut realip_remote_addr = None;

src/request.rs

@@ -38,6 +38,7 @@ pub(crate) struct HttpRequestInner {
     pub(crate) path: Path<Url>,
     pub(crate) app_data: SmallVec<[Rc<Extensions>; 4]>,
     pub(crate) conn_data: Option<Rc<Extensions>>,
+    pub(crate) req_data: Rc<RefCell<Extensions>>,
     app_state: Rc<AppInitServiceState>,
 }
 
@@ -49,6 +50,7 @@ impl HttpRequest {
         app_state: Rc<AppInitServiceState>,
         app_data: Rc<Extensions>,
         conn_data: Option<Rc<Extensions>>,
+        req_data: Rc<RefCell<Extensions>>,
     ) -> HttpRequest {
         let mut data = SmallVec::<[Rc<Extensions>; 4]>::new();
         data.push(app_data);
@@ -60,6 +62,7 @@ impl HttpRequest {
                 app_state,
                 app_data: data,
                 conn_data,
+                req_data,
             }),
         }
     }
@@ -156,16 +159,12 @@ impl HttpRequest {
         self.resource_map().match_name(self.path())
     }
 
-    /// Request extensions
+    pub fn req_data(&self) -> Ref<'_, Extensions> {
-    #[inline]
+        self.inner.req_data.borrow()
-    pub fn extensions(&self) -> Ref<'_, Extensions> {
-        self.head().extensions()
     }
 
-    /// Mutable reference to a the request's extensions
+    pub fn req_data_mut(&self) -> RefMut<'_, Extensions> {
-    #[inline]
+        self.inner.req_data.borrow_mut()
-    pub fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.head().extensions_mut()
     }
 
     /// Returns a reference a piece of connection data set in an [on-connect] callback.
@@ -248,7 +247,12 @@ impl HttpRequest {
     /// borrowed.
     #[inline]
     pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> {
-        ConnectionInfo::get(self.head(), self.app_config())
+        if !self.extensions().contains::<ConnectionInfo>() {
+            let info = ConnectionInfo::new(self.head(), &*self.app_config());
+            self.extensions_mut().insert(info);
+        }
+
+        Ref::map(self.extensions(), |e| e.get().unwrap())
     }
 
     /// App config
@@ -321,21 +325,18 @@ impl HttpMessage for HttpRequest {
     type Stream = ();
 
     #[inline]
-    /// Returns Request's headers.
     fn headers(&self) -> &HeaderMap {
         &self.head().headers
     }
 
-    /// Request extensions
     #[inline]
     fn extensions(&self) -> Ref<'_, Extensions> {
-        self.inner.head.extensions()
+        self.req_data()
     }
 
-    /// Mutable reference to a the request's extensions
     #[inline]
     fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.inner.head.extensions_mut()
+        self.req_data_mut()
     }
 
     #[inline]
@@ -348,14 +349,15 @@ impl Drop for HttpRequest {
     fn drop(&mut self) {
         // if possible, contribute to current worker's HttpRequest allocation pool
 
-        // This relies on no Weak<HttpRequestInner> exists anywhere.(There is none)
+        // This relies on no Weak<HttpRequestInner> exists anywhere. (There is none.)
         if let Some(inner) = Rc::get_mut(&mut self.inner) {
             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 is borrowed mut here and; get req data mutably to reduce borrow check. Also
-                inner.head.extensions.get_mut().clear();
+                // we know the req_data Rc will not have any cloned at this point to unwrap is okay.
+                Rc::get_mut(&mut inner.req_data).unwrap().get_mut().clear();
 
                 // a re-borrow of pool is necessary here.
                 let req = self.inner.clone();

src/request_data.rs

@@ -33,10 +33,9 @@ use crate::{dev::Payload, error::ErrorInternalServerError, Error, FromRequest, H
 ///     req: HttpRequest,
 ///     opt_flag: Option<web::ReqData<FlagFromMiddleware>>,
 /// ) -> impl Responder {
-///     // use an optional extractor if the middleware is
+///     // use an option extractor if middleware is not guaranteed to add this type of req data
-///     // not guaranteed to add this type of requests data
 ///     if let Some(flag) = opt_flag {
-///         assert_eq!(&flag.into_inner(), req.extensions().get::<FlagFromMiddleware>().unwrap());
+///         assert_eq!(&flag.into_inner(), req.req_data().get::<FlagFromMiddleware>().unwrap());
 ///     }
 ///
 ///     HttpResponse::Ok()
@@ -68,7 +67,7 @@ impl<T: Clone + 'static> FromRequest for ReqData<T> {
     type Future = Ready<Result<Self, Error>>;
 
     fn from_request(req: &HttpRequest, _: &mut Payload) -> Self::Future {
-        if let Some(st) = req.extensions().get::<T>() {
+        if let Some(st) = req.req_data().get::<T>() {
             ok(ReqData(st.clone()))
         } else {
             log::debug!(

src/service.rs

@@ -194,7 +194,7 @@ impl ServiceRequest {
     /// Get *ConnectionInfo* for the current request.
     #[inline]
     pub fn connection_info(&self) -> Ref<'_, ConnectionInfo> {
-        ConnectionInfo::get(self.head(), &*self.app_config())
+        self.req.connection_info()
     }
 
     /// Get a reference to the Path parameters.

src/test.rs

@@ -581,7 +581,14 @@ impl TestRequest {
         let app_state = AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
         ServiceRequest::new(
-            HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data), None),
+            HttpRequest::new(
+                self.path,
+                head,
+                app_state,
+                Rc::new(self.app_data),
+                None,
+                Default::default(),
+            ),
             payload,
         )
     }
@@ -599,7 +606,14 @@ impl TestRequest {
 
         let app_state = AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
-        HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data), None)
+        HttpRequest::new(
+            self.path,
+            head,
+            app_state,
+            Rc::new(self.app_data),
+            None,
+            Default::default(),
+        )
     }
 
     /// Complete request creation and generate `HttpRequest` and `Payload` instances
@@ -610,7 +624,14 @@ impl TestRequest {
 
         let app_state = AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
-        let req = HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data), None);
+        let req = HttpRequest::new(
+            self.path,
+            head,
+            app_state,
+            Rc::new(self.app_data),
+            None,
+            Default::default(),
+        );
 
         (req, payload)
     }