diff --git a/CHANGES.md b/CHANGES.md index 70f7705c8..00608df76 100644 --- a/CHANGES.md +++ b/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 diff --git a/src/app_service.rs b/src/app_service.rs index c4ac0b094..8169be517 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}; @@ -201,9 +201,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(); @@ -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)) } } diff --git a/src/request.rs b/src/request.rs index 437d07b6e..c0e26006c 100644 --- a/src/request.rs +++ b/src/request.rs @@ -28,7 +28,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]>, app_state: Rc, } @@ -38,7 +37,6 @@ impl HttpRequest { pub(crate) fn new( path: Path, head: Message, - payload: Payload, app_state: Rc, app_data: Rc, ) -> HttpRequest { @@ -49,7 +47,6 @@ impl HttpRequest { inner: Rc::new(HttpRequestInner { head, path, - payload, app_state, app_data: data, }), diff --git a/src/service.rs b/src/service.rs index c6a961efc..668b7d1b4 100644 --- a/src/service.rs +++ b/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 { - 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 +182,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 +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) { - 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 { - 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( diff --git a/src/test.rs b/src/test.rs index 271ed4505..f8b789d1b 100644 --- a/src/test.rs +++ b/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) }