optimize ServiceRequest methods (#1870)

Co-authored-by: Rob Ede <robjtede@icloud.com>

src/app_service.rs

@@ -238,7 +238,7 @@ where
         let (head, payload) = req.into_parts();
 
         let req = if let Some(mut req) = self.pool.get_request() {
-            let inner = Rc::get_mut(&mut req.0).unwrap();
+            let inner = Rc::get_mut(&mut req.inner).unwrap();
             inner.path.get_mut().update(&head.uri);
             inner.path.reset();
             inner.head = head;

src/request.rs

@@ -16,7 +16,12 @@ use crate::rmap::ResourceMap;
 
 #[derive(Clone)]
 /// An HTTP Request
-pub struct HttpRequest(pub(crate) Rc<HttpRequestInner>);
+pub struct HttpRequest {
+    // *. Rc<HttpRequestInner> is used exclusively and NO Weak<HttpRequestInner>
+    // is allowed anywhere in the code. Weak pointer is purposely ignored when
+    // doing Rc's ref counter check.
+    pub(crate) inner: Rc<HttpRequestInner>,
+}
 
 pub(crate) struct HttpRequestInner {
     pub(crate) head: Message<RequestHead>,
@@ -42,15 +47,17 @@ impl HttpRequest {
         let mut data = SmallVec::<[Rc<Extensions>; 4]>::new();
         data.push(app_data);
 
-        HttpRequest(Rc::new(HttpRequestInner {
+        HttpRequest {
-            head,
+            inner: Rc::new(HttpRequestInner {
-            path,
+                head,
-            payload,
+                path,
-            rmap,
+                payload,
-            config,
+                rmap,
-            app_data: data,
+                config,
-            pool,
+                app_data: data,
-        }))
+                pool,
+            }),
+        }
     }
 }
 
@@ -58,14 +65,14 @@ impl HttpRequest {
     /// This method returns reference to the request head
     #[inline]
     pub fn head(&self) -> &RequestHead {
-        &self.0.head
+        &self.inner.head
     }
 
     /// This method returns mutable reference to the request head.
     /// panics if multiple references of http request exists.
     #[inline]
     pub(crate) fn head_mut(&mut self) -> &mut RequestHead {
-        &mut Rc::get_mut(&mut self.0).unwrap().head
+        &mut Rc::get_mut(&mut self.inner).unwrap().head
     }
 
     /// Request's uri.
@@ -118,12 +125,12 @@ impl HttpRequest {
     /// access the matched value for that segment.
     #[inline]
     pub fn match_info(&self) -> &Path<Url> {
-        &self.0.path
+        &self.inner.path
     }
 
     #[inline]
     pub(crate) fn match_info_mut(&mut self) -> &mut Path<Url> {
-        &mut Rc::get_mut(&mut self.0).unwrap().path
+        &mut Rc::get_mut(&mut self.inner).unwrap().path
     }
 
     /// The resource definition pattern that matched the path. Useful for logging and metrics.
@@ -134,7 +141,7 @@ impl HttpRequest {
     /// Returns a None when no resource is fully matched, including default services.
     #[inline]
     pub fn match_pattern(&self) -> Option<String> {
-        self.0.rmap.match_pattern(self.path())
+        self.inner.rmap.match_pattern(self.path())
     }
 
     /// The resource name that matched the path. Useful for logging and metrics.
@@ -142,7 +149,7 @@ impl HttpRequest {
     /// Returns a None when no resource is fully matched, including default services.
     #[inline]
     pub fn match_name(&self) -> Option<&str> {
-        self.0.rmap.match_name(self.path())
+        self.inner.rmap.match_name(self.path())
     }
 
     /// Request extensions
@@ -184,7 +191,7 @@ impl HttpRequest {
         U: IntoIterator<Item = I>,
         I: AsRef<str>,
     {
-        self.0.rmap.url_for(&self, name, elements)
+        self.inner.rmap.url_for(&self, name, elements)
     }
 
     /// Generate url for named resource
@@ -199,7 +206,7 @@ impl HttpRequest {
     #[inline]
     /// Get a reference to a `ResourceMap` of current application.
     pub fn resource_map(&self) -> &ResourceMap {
-        &self.0.rmap
+        &self.inner.rmap
     }
 
     /// Peer socket address
@@ -225,7 +232,7 @@ impl HttpRequest {
     /// App config
     #[inline]
     pub fn app_config(&self) -> &AppConfig {
-        &self.0.config
+        &self.inner.config
     }
 
     /// Get an application data object stored with `App::data` or `App::app_data`
@@ -237,7 +244,7 @@ impl HttpRequest {
     /// let opt_t = req.app_data::<Data<T>>();
     /// ```
     pub fn app_data<T: 'static>(&self) -> Option<&T> {
-        for container in self.0.app_data.iter().rev() {
+        for container in self.inner.app_data.iter().rev() {
             if let Some(data) = container.get::<T>() {
                 return Some(data);
             }
@@ -259,13 +266,13 @@ impl HttpMessage for HttpRequest {
     /// Request extensions
     #[inline]
     fn extensions(&self) -> Ref<'_, Extensions> {
-        self.0.head.extensions()
+        self.inner.head.extensions()
     }
 
     /// Mutable reference to a the request's extensions
     #[inline]
     fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.0.head.extensions_mut()
+        self.inner.head.extensions_mut()
     }
 
     #[inline]
@@ -279,7 +286,7 @@ impl Drop for HttpRequest {
         // if possible, contribute to current worker's HttpRequest allocation pool
 
         // This relies on no Weak<HttpRequestInner> exists anywhere.(There is none)
-        if let Some(inner) = Rc::get_mut(&mut self.0) {
+        if let Some(inner) = Rc::get_mut(&mut self.inner) {
             let v = &mut inner.pool.0.borrow_mut();
             if v.len() < 128 {
                 // clear additional app_data and keep the root one for reuse.
@@ -287,7 +294,7 @@ impl Drop for HttpRequest {
                 // inner is borrowed mut here. get head's Extension mutably
                 // to reduce borrow check
                 inner.head.extensions.get_mut().clear();
-                v.push(self.0.clone());
+                v.push(self.inner.clone());
             }
         }
     }
@@ -329,8 +336,8 @@ impl fmt::Debug for HttpRequest {
         writeln!(
             f,
             "\nHttpRequest {:?} {}:{}",
-            self.0.head.version,
+            self.inner.head.version,
-            self.0.head.method,
+            self.inner.head.method,
             self.path()
         )?;
         if !self.query_string().is_empty() {
@@ -369,7 +376,7 @@ impl HttpRequestPool {
     /// Re-use a previously allocated (but now completed/discarded) HttpRequest object.
     #[inline]
     pub(crate) fn get_request(&self) -> Option<HttpRequest> {
-        self.0.borrow_mut().pop().map(HttpRequest)
+        self.0.borrow_mut().pop().map(|inner| HttpRequest { inner })
     }
 
     /// Clears all allocated HttpRequest objects.

src/service.rs

@@ -62,7 +62,7 @@ impl ServiceRequest {
 
     /// Deconstruct request into parts
     pub fn into_parts(mut self) -> (HttpRequest, Payload) {
-        let pl = Rc::get_mut(&mut (self.0).0).unwrap().payload.take();
+        let pl = Rc::get_mut(&mut (self.0).inner).unwrap().payload.take();
         (self.0, pl)
     }
 
@@ -73,11 +73,12 @@ impl ServiceRequest {
         mut req: HttpRequest,
         pl: Payload,
     ) -> Result<Self, (HttpRequest, Payload)> {
-        if Rc::strong_count(&req.0) == 1 && Rc::weak_count(&req.0) == 0 {
+        match Rc::get_mut(&mut req.inner) {
-            Rc::get_mut(&mut req.0).unwrap().payload = pl;
+            Some(p) => {
-            Ok(ServiceRequest(req))
+                p.payload = pl;
-        } else {
+                Ok(ServiceRequest(req))
-            Err((req, pl))
+            }
+            None => Err((req, pl)),
         }
     }
 
@@ -87,7 +88,10 @@ impl ServiceRequest {
     /// can be re-constructed only if rc's strong pointers count eq 1 and
     /// weak pointers count is 0.
     pub fn from_request(req: HttpRequest) -> Result<Self, HttpRequest> {
-        if Rc::strong_count(&req.0) == 1 && Rc::weak_count(&req.0) == 0 {
+        // There is no weak pointer used on HttpRequest so intentionally
+        // ignore the check.
+        if Rc::strong_count(&req.inner) == 1 {
+            debug_assert!(Rc::weak_count(&req.inner) == 0);
             Ok(ServiceRequest(req))
         } else {
             Err(req)
@@ -227,7 +231,7 @@ impl ServiceRequest {
 
     /// Counterpart to [`HttpRequest::app_data`](super::HttpRequest::app_data()).
     pub fn app_data<T: 'static>(&self) -> Option<&T> {
-        for container in (self.0).0.app_data.iter().rev() {
+        for container in (self.0).inner.app_data.iter().rev() {
             if let Some(data) = container.get::<T>() {
                 return Some(data);
             }
@@ -238,13 +242,13 @@ impl ServiceRequest {
 
     /// Set request payload.
     pub fn set_payload(&mut self, payload: Payload) {
-        Rc::get_mut(&mut (self.0).0).unwrap().payload = payload;
+        Rc::get_mut(&mut (self.0).inner).unwrap().payload = payload;
     }
 
     #[doc(hidden)]
     /// Add app data container to request's resolution set.
     pub fn add_data_container(&mut self, extensions: Rc<Extensions>) {
-        Rc::get_mut(&mut (self.0).0)
+        Rc::get_mut(&mut (self.0).inner)
             .unwrap()
             .app_data
             .push(extensions);
@@ -280,7 +284,7 @@ impl HttpMessage for ServiceRequest {
 
     #[inline]
     fn take_payload(&mut self) -> Payload<Self::Stream> {
-        Rc::get_mut(&mut (self.0).0).unwrap().payload.take()
+        Rc::get_mut(&mut (self.0).inner).unwrap().payload.take()
     }
 }