From 7db0ded43bf0cb1da41370f41fd0323d60978bd0 Mon Sep 17 00:00:00 2001 From: fakeshadow <24548779@qq.com> Date: Sat, 9 Jan 2021 10:47:27 +0800 Subject: [PATCH] move payload type out of HttpRequest --- CHANGES.md | 3 ++ src/app_service.rs | 10 ++--- src/request.rs | 3 -- src/service.rs | 104 ++++++++++++++++++++------------------------- src/test.rs | 22 +++++----- 5 files changed, 63 insertions(+), 79 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 4eb2b6e1b..06fc995a5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,8 +4,11 @@ ### 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] [#1891]: https://github.com/actix/actix-web/pull/1891 +[#1893]: https://github.com/actix/actix-web/pull/1893 ## 4.0.0-beta.1 - 2021-01-07 ### Added diff --git a/src/app_service.rs b/src/app_service.rs index 442de9362..e9c53e3e2 100644 --- a/src/app_service.rs +++ b/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}; @@ -170,9 +170,7 @@ where type Error = T::Error; type Future = T::Future; - fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - self.service.poll_ready(cx) - } + actix_service::forward_ready!(service); fn call(&mut self, req: Request) -> Self::Future { let (head, payload) = req.into_parts(); @@ -182,20 +180,18 @@ 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.rmap.clone(), self.config.clone(), self.app_data.clone(), self.pool, ) }; - self.service.call(ServiceRequest::new(req)) + self.service.call(ServiceRequest::new(req, payload)) } } diff --git a/src/request.rs b/src/request.rs index 82304c1af..7aab68148 100644 --- a/src/request.rs +++ b/src/request.rs @@ -26,7 +26,6 @@ pub struct HttpRequest { pub(crate) struct HttpRequestInner { pub(crate) head: Message, pub(crate) path: Path, - pub(crate) payload: Payload, pub(crate) app_data: SmallVec<[Rc; 4]>, rmap: Rc, config: AppConfig, @@ -38,7 +37,6 @@ impl HttpRequest { pub(crate) fn new( path: Path, head: Message, - payload: Payload, rmap: Rc, config: AppConfig, app_data: Rc, @@ -51,7 +49,6 @@ impl HttpRequest { inner: Rc::new(HttpRequestInner { head, path, - payload, rmap, config, app_data: data, diff --git a/src/service.rs b/src/service.rs index c6a961efc..c78a62627 100644 --- a/src/service.rs +++ b/src/service.rs @@ -52,75 +52,63 @@ 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 { - 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 { - // 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>>(self, res: R) -> ServiceResponse { - ServiceResponse::new(self.0, res.into()) + ServiceResponse::new(self.req, res.into()) } /// Create service response for error #[inline] pub fn error_response>(self, err: E) -> ServiceResponse { 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 +184,42 @@ impl ServiceRequest { /// access the matched value for that segment. #[inline] pub fn match_info(&self) -> &Path { - 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 { - 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 { - 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(&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::() { return Some(data); } @@ -242,13 +230,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) { - Rc::get_mut(&mut (self.0).inner) + Rc::get_mut(&mut (self.req).inner) .unwrap() .app_data .push(extensions); @@ -273,18 +261,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 { - Rc::get_mut(&mut (self.0).inner).unwrap().payload.take() + self.payload.take() } } @@ -554,23 +542,23 @@ mod tests { #[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(); + // 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(); + // 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(); + // 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()); + // 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] diff --git a/src/test.rs b/src/test.rs index a76bae6a6..bdd62e9d7 100644 --- a/src/test.rs +++ b/src/test.rs @@ -542,15 +542,17 @@ impl TestRequest { head.peer_addr = self.peer_addr; self.path.get_mut().update(&head.uri); - ServiceRequest::new(HttpRequest::new( - self.path, - head, + ServiceRequest::new( + HttpRequest::new( + self.path, + head, + Rc::new(self.rmap), + self.config.clone(), + Rc::new(self.app_data), + HttpRequestPool::create(), + ), payload, - Rc::new(self.rmap), - self.config.clone(), - Rc::new(self.app_data), - HttpRequestPool::create(), - )) + ) } /// Complete request creation and generate `ServiceResponse` instance @@ -560,14 +562,13 @@ 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); HttpRequest::new( self.path, head, - payload, Rc::new(self.rmap), self.config.clone(), Rc::new(self.app_data), @@ -584,7 +585,6 @@ impl TestRequest { let req = HttpRequest::new( self.path, head, - Payload::None, Rc::new(self.rmap), self.config.clone(), Rc::new(self.app_data),