Refactor/service request (#1893)

CHANGES.md

@@ -8,6 +8,8 @@
 ### Changed
 * Rework `Responder` trait to be sync and returns `Response`/`HttpResponse` directly.
   Making it more simple and performant. [#1891]
+* `ServiceRequest::into_parts` and `ServiceRequest::from_parts` would not fail.
+  `ServiceRequest::from_request` would not fail and no payload would be generated [#1893]
 * Our `Either` type now uses `Left`/`Right` variants (instead of `A`/`B`) [#1894]
 
 ### Removed
@@ -15,8 +17,10 @@
 * Public field of `web::Query` has been made private. [#1894]
 
 [#1891]: https://github.com/actix/actix-web/pull/1891
+[#1893]: https://github.com/actix/actix-web/pull/1893
 [#1894]: https://github.com/actix/actix-web/pull/1894
 
+
 ## 4.0.0-beta.1 - 2021-01-07
 ### Added
 * `Compat` middleware enabling generic response body/error type of middlewares like `Logger` and

src/app_service.rs

@@ -1,6 +1,6 @@
 use std::cell::RefCell;
 use std::rc::Rc;
-use std::task::{Context, Poll};
+use std::task::Poll;
 
 use actix_http::{Extensions, Request, Response};
 use actix_router::{Path, ResourceDef, Router, Url};
@@ -201,9 +201,7 @@ where
     type Error = T::Error;
     type Future = T::Future;
 
-    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
-        self.service.poll_ready(cx)
-    }
+    actix_service::forward_ready!(service);
 
     fn call(&mut self, req: Request) -> Self::Future {
         let (head, payload) = req.into_parts();
@@ -213,18 +211,16 @@ where
             inner.path.get_mut().update(&head.uri);
             inner.path.reset();
             inner.head = head;
-            inner.payload = payload;
             req
         } else {
             HttpRequest::new(
                 Path::new(Url::new(head.uri.clone())),
                 head,
-                payload,
                 self.app_state.clone(),
                 self.app_data.clone(),
             )
         };
-        self.service.call(ServiceRequest::new(req))
+        self.service.call(ServiceRequest::new(req, payload))
     }
 }
 

src/request.rs

@@ -28,7 +28,6 @@ pub struct HttpRequest {
 pub(crate) struct HttpRequestInner {
     pub(crate) head: Message<RequestHead>,
     pub(crate) path: Path<Url>,
-    pub(crate) payload: Payload,
     pub(crate) app_data: SmallVec<[Rc<Extensions>; 4]>,
     app_state: Rc<AppInitServiceState>,
 }
@@ -38,7 +37,6 @@ impl HttpRequest {
     pub(crate) fn new(
         path: Path<Url>,
         head: Message<RequestHead>,
-        payload: Payload,
         app_state: Rc<AppInitServiceState>,
         app_data: Rc<Extensions>,
     ) -> HttpRequest {
@@ -49,7 +47,6 @@ impl HttpRequest {
             inner: Rc::new(HttpRequestInner {
                 head,
                 path,
-                payload,
                 app_state,
                 app_data: data,
             }),

src/service.rs

@@ -52,75 +52,61 @@ where
 /// An service http request
 ///
 /// ServiceRequest allows mutable access to request's internal structures
-pub struct ServiceRequest(HttpRequest);
+pub struct ServiceRequest {
+    req: HttpRequest,
+    payload: Payload,
+}
 
 impl ServiceRequest {
     /// Construct service request
-    pub(crate) fn new(req: HttpRequest) -> Self {
-        ServiceRequest(req)
+    pub(crate) fn new(req: HttpRequest, payload: Payload) -> Self {
+        Self { req, payload }
     }
 
     /// Deconstruct request into parts
-    pub fn into_parts(mut self) -> (HttpRequest, Payload) {
-        let pl = Rc::get_mut(&mut (self.0).inner).unwrap().payload.take();
-        (self.0, pl)
+    #[inline]
+    pub fn into_parts(self) -> (HttpRequest, Payload) {
+        (self.req, self.payload)
     }
 
     /// Construct request from parts.
-    ///
-    /// `ServiceRequest` can be re-constructed only if `req` hasn't been cloned.
-    pub fn from_parts(
-        mut req: HttpRequest,
-        pl: Payload,
-    ) -> Result<Self, (HttpRequest, Payload)> {
-        match Rc::get_mut(&mut req.inner) {
-            Some(p) => {
-                p.payload = pl;
-                Ok(ServiceRequest(req))
-            }
-            None => Err((req, pl)),
-        }
+    pub fn from_parts(req: HttpRequest, payload: Payload) -> Self {
+        Self { req, payload }
     }
 
     /// Construct request from request.
     ///
-    /// `HttpRequest` implements `Clone` trait via `Rc` type. `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> {
-        // 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)
+    /// The returned `ServiceRequest` would have no payload.
+    pub fn from_request(req: HttpRequest) -> Self {
+        ServiceRequest {
+            req,
+            payload: Payload::None,
         }
     }
 
     /// Create service response
     #[inline]
     pub fn into_response<B, R: Into<Response<B>>>(self, res: R) -> ServiceResponse<B> {
-        ServiceResponse::new(self.0, res.into())
+        ServiceResponse::new(self.req, res.into())
     }
 
     /// Create service response for error
     #[inline]
     pub fn error_response<B, E: Into<Error>>(self, err: E) -> ServiceResponse<B> {
         let res: Response = err.into().into();
-        ServiceResponse::new(self.0, res.into_body())
+        ServiceResponse::new(self.req, res.into_body())
     }
 
     /// This method returns reference to the request head
     #[inline]
     pub fn head(&self) -> &RequestHead {
-        &self.0.head()
+        &self.req.head()
     }
 
     /// This method returns reference to the request head
     #[inline]
     pub fn head_mut(&mut self) -> &mut RequestHead {
-        self.0.head_mut()
+        self.req.head_mut()
     }
 
     /// Request's uri.
@@ -196,42 +182,42 @@ impl ServiceRequest {
     /// access the matched value for that segment.
     #[inline]
     pub fn match_info(&self) -> &Path<Url> {
-        self.0.match_info()
+        self.req.match_info()
     }
 
     /// Counterpart to [`HttpRequest::match_name`](super::HttpRequest::match_name()).
     #[inline]
     pub fn match_name(&self) -> Option<&str> {
-        self.0.match_name()
+        self.req.match_name()
     }
 
     /// Counterpart to [`HttpRequest::match_pattern`](super::HttpRequest::match_pattern()).
     #[inline]
     pub fn match_pattern(&self) -> Option<String> {
-        self.0.match_pattern()
+        self.req.match_pattern()
     }
 
     #[inline]
     /// Get a mutable reference to the Path parameters.
     pub fn match_info_mut(&mut self) -> &mut Path<Url> {
-        self.0.match_info_mut()
+        self.req.match_info_mut()
     }
 
     #[inline]
     /// Get a reference to a `ResourceMap` of current application.
     pub fn resource_map(&self) -> &ResourceMap {
-        self.0.resource_map()
+        self.req.resource_map()
     }
 
     /// Service configuration
     #[inline]
     pub fn app_config(&self) -> &AppConfig {
-        self.0.app_config()
+        self.req.app_config()
     }
 
     /// Counterpart to [`HttpRequest::app_data`](super::HttpRequest::app_data()).
     pub fn app_data<T: 'static>(&self) -> Option<&T> {
-        for container in (self.0).inner.app_data.iter().rev() {
+        for container in self.req.inner.app_data.iter().rev() {
             if let Some(data) = container.get::<T>() {
                 return Some(data);
             }
@@ -242,13 +228,13 @@ impl ServiceRequest {
 
     /// Set request payload.
     pub fn set_payload(&mut self, payload: Payload) {
-        Rc::get_mut(&mut (self.0).inner).unwrap().payload = payload;
+        self.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).inner)
+        Rc::get_mut(&mut (self.req).inner)
             .unwrap()
             .app_data
             .push(extensions);
@@ -273,18 +259,18 @@ impl HttpMessage for ServiceRequest {
     /// Request extensions
     #[inline]
     fn extensions(&self) -> Ref<'_, Extensions> {
-        self.0.extensions()
+        self.req.extensions()
     }
 
     /// Mutable reference to a the request's extensions
     #[inline]
     fn extensions_mut(&self) -> RefMut<'_, Extensions> {
-        self.0.extensions_mut()
+        self.req.extensions_mut()
     }
 
     #[inline]
     fn take_payload(&mut self) -> Payload<Self::Stream> {
-        Rc::get_mut(&mut (self.0).inner).unwrap().payload.take()
+        self.payload.take()
     }
 }
 
@@ -552,27 +538,6 @@ mod tests {
     use actix_service::Service;
     use futures_util::future::ok;
 
-    #[test]
-    fn test_service_request() {
-        let req = TestRequest::default().to_srv_request();
-        let (r, pl) = req.into_parts();
-        assert!(ServiceRequest::from_parts(r, pl).is_ok());
-
-        let req = TestRequest::default().to_srv_request();
-        let (r, pl) = req.into_parts();
-        let _r2 = r.clone();
-        assert!(ServiceRequest::from_parts(r, pl).is_err());
-
-        let req = TestRequest::default().to_srv_request();
-        let (r, _pl) = req.into_parts();
-        assert!(ServiceRequest::from_request(r).is_ok());
-
-        let req = TestRequest::default().to_srv_request();
-        let (r, _pl) = req.into_parts();
-        let _r2 = r.clone();
-        assert!(ServiceRequest::from_request(r).is_err());
-    }
-
     #[actix_rt::test]
     async fn test_service() {
         let mut srv = init_service(

src/test.rs

@@ -545,13 +545,10 @@ impl TestRequest {
         let app_state =
             AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
-        ServiceRequest::new(HttpRequest::new(
-            self.path,
-            head,
+        ServiceRequest::new(
+            HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data)),
             payload,
-            app_state,
-            Rc::new(self.app_data),
-        ))
+        )
     }
 
     /// Complete request creation and generate `ServiceResponse` instance
@@ -561,14 +558,14 @@ impl TestRequest {
 
     /// Complete request creation and generate `HttpRequest` instance
     pub fn to_http_request(mut self) -> HttpRequest {
-        let (mut head, payload) = self.req.finish().into_parts();
+        let (mut head, _) = self.req.finish().into_parts();
         head.peer_addr = self.peer_addr;
         self.path.get_mut().update(&head.uri);
 
         let app_state =
             AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
-        HttpRequest::new(self.path, head, payload, app_state, Rc::new(self.app_data))
+        HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data))
     }
 
     /// Complete request creation and generate `HttpRequest` and `Payload` instances
@@ -580,13 +577,7 @@ impl TestRequest {
         let app_state =
             AppInitServiceState::new(Rc::new(self.rmap), self.config.clone());
 
-        let req = HttpRequest::new(
-            self.path,
-            head,
-            Payload::None,
-            app_state,
-            Rc::new(self.app_data),
-        );
+        let req = HttpRequest::new(self.path, head, app_state, Rc::new(self.app_data));
 
         (req, payload)
     }