From ce40201ab5fafbe0572840ba5b343cfac5f97783 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sun, 9 May 2021 02:56:05 +0100 Subject: [PATCH] remove response future impl --- actix-http/CHANGES.md | 1 + actix-http/src/message.rs | 4 +- actix-http/src/response.rs | 124 +++++++++++------------------ actix-http/src/response_builder.rs | 2 +- src/response/response.rs | 25 +++--- tests/test_server.rs | 2 +- 6 files changed, 65 insertions(+), 93 deletions(-) diff --git a/actix-http/CHANGES.md b/actix-http/CHANGES.md index 51598288a..595d3a924 100644 --- a/actix-http/CHANGES.md +++ b/actix-http/CHANGES.md @@ -19,6 +19,7 @@ * Stop re-exporting `http` crate's `HeaderMap` types in addition to ours. [#2171] * Down-casting for `MessageBody` types. [#2183] * `error::Result` alias. [#2201] +* `impl Future` for `Response`. [#2201] [#2171]: https://github.com/actix/actix-web/pull/2171 [#2183]: https://github.com/actix/actix-web/pull/2183 diff --git a/actix-http/src/message.rs b/actix-http/src/message.rs index 5ce40ee8e..bf498b32c 100644 --- a/actix-http/src/message.rs +++ b/actix-http/src/message.rs @@ -293,14 +293,14 @@ impl ResponseHead { } } - #[inline] /// Check if keep-alive is enabled + #[inline] pub fn keep_alive(&self) -> bool { self.connection_type() == ConnectionType::KeepAlive } - #[inline] /// Check upgrade status of this message + #[inline] pub fn upgrade(&self) -> bool { self.connection_type() == ConnectionType::Upgrade } diff --git a/actix-http/src/response.rs b/actix-http/src/response.rs index 900c4d22e..427b2025e 100644 --- a/actix-http/src/response.rs +++ b/actix-http/src/response.rs @@ -19,22 +19,22 @@ use crate::{ /// An HTTP response. pub struct Response { pub(crate) head: BoxedResponseHead, - pub(crate) body: Option, + pub(crate) body: B, pub(crate) error: Option, } impl Response { - /// Constructs a response + /// Constructs a new response with default body. #[inline] pub fn new(status: StatusCode) -> Response { Response { head: BoxedResponseHead::new(status), - body: Some(Body::Empty), + body: Body::Empty, error: None, } } - /// Create HTTP response builder with specific status. + /// Constructs a new response builder. #[inline] pub fn build(status: StatusCode) -> ResponseBuilder { ResponseBuilder::new(status) @@ -43,25 +43,25 @@ impl Response { // just a couple frequently used shortcuts // this list should not grow larger than a few - /// Creates a new response with status 200 OK. + /// Constructs a new response with status 200 OK. #[inline] pub fn ok() -> Response { Response::new(StatusCode::OK) } - /// Creates a new response with status 400 Bad Request. + /// Constructs a new response with status 400 Bad Request. #[inline] pub fn bad_request() -> Response { Response::new(StatusCode::BAD_REQUEST) } - /// Creates a new response with status 404 Not Found. + /// Constructs a new response with status 404 Not Found. #[inline] pub fn not_found() -> Response { Response::new(StatusCode::NOT_FOUND) } - /// Creates a new response with status 500 Internal Server Error. + /// Constructs a new response with status 500 Internal Server Error. #[inline] pub fn internal_server_error() -> Response { Response::new(StatusCode::INTERNAL_SERVER_ERROR) @@ -69,7 +69,7 @@ impl Response { // end shortcuts - /// Constructs an error response + /// Constructs a new response from an error. #[inline] pub fn from_error(error: Error) -> Response { let mut resp = error.as_response_error().error_response(); @@ -82,146 +82,139 @@ impl Response { } impl Response { - /// Constructs a response with body + /// Constructs a new response with given body. #[inline] pub fn with_body(status: StatusCode, body: B) -> Response { Response { head: BoxedResponseHead::new(status), - body: Some(body), + body: body, error: None, } } + /// Return a reference to the head of this response. #[inline] - /// Http message part of the response pub fn head(&self) -> &ResponseHead { &*self.head } + /// Return a mutable reference to the head of this response. #[inline] - /// Mutable reference to a HTTP message part of the response pub fn head_mut(&mut self) -> &mut ResponseHead { &mut *self.head } - /// The source `error` for this response + /// Return the source `error` for this response, if one is set. #[inline] pub fn error(&self) -> Option<&Error> { self.error.as_ref() } - /// Get the response status code + /// Return the status code of this response. #[inline] pub fn status(&self) -> StatusCode { self.head.status } - /// Set the `StatusCode` for this response + /// Returns a mutable reference the status code of this response. #[inline] pub fn status_mut(&mut self) -> &mut StatusCode { &mut self.head.status } - /// Get the headers from the response + /// Returns a reference to response headers. #[inline] pub fn headers(&self) -> &HeaderMap { &self.head.headers } - /// Get a mutable reference to the headers + /// Returns a mutable reference to response headers. #[inline] pub fn headers_mut(&mut self) -> &mut HeaderMap { &mut self.head.headers } - /// Connection upgrade status + /// Returns true if connection upgrade is enabled. #[inline] pub fn upgrade(&self) -> bool { self.head.upgrade() } - /// Keep-alive status for this connection + /// Returns true if keep-alive is enabled. pub fn keep_alive(&self) -> bool { self.head.keep_alive() } - /// Responses extensions + /// Returns a reference to the extensions of this response. #[inline] pub fn extensions(&self) -> Ref<'_, Extensions> { self.head.extensions.borrow() } - /// Mutable reference to a the response's extensions + /// Returns a mutable reference to the extensions of this response. #[inline] pub fn extensions_mut(&mut self) -> RefMut<'_, Extensions> { self.head.extensions.borrow_mut() } - /// Get body of this response + /// Returns a reference to the body of this response. #[inline] pub fn body(&self) -> &B { - self.body.as_ref().unwrap() + &self.body } - /// Set a body + /// Sets new body. pub fn set_body(self, body: B2) -> Response { Response { head: self.head, - body: Some(body), + body, error: None, } } - /// Split response and body - pub fn into_parts(self) -> (Response<()>, B) { - ( - Response { - head: self.head, - body: Some(()), - error: self.error, - }, - self.body.unwrap(), - ) - } - - /// Drop request's body + /// Drops body and returns new response. pub fn drop_body(self) -> Response<()> { - Response { - head: self.head, - body: Some(()), - error: None, - } + self.set_body(()) } - /// Set a body and return previous body value + /// Sets new body, returning new response and previous body value. pub(crate) fn replace_body(self, body: B2) -> (Response, B) { ( Response { head: self.head, - body: Some(body), + body, error: self.error, }, - self.body.unwrap(), + self.body, ) } - /// Set a body and return previous body value + /// Returns split head and body. + /// + /// # Implementation Notes + /// Due to internal performance optimisations, the first element of the returned tuple is a + /// `Response` as well but only contains the head of the response this was called on. + pub fn into_parts(self) -> (Response<()>, B) { + self.replace_body(()) + } + + /// Returns new response with mapped body. pub fn map_body(mut self, f: F) -> Response where F: FnOnce(&mut ResponseHead, B) -> B2, { - let body = f(&mut self.head, self.body.unwrap()); + let body = f(&mut self.head, self.body); Response { head: self.head, - body: Some(body), + body, error: self.error, } } - /// Extract response body + /// Returns body, consuming this response. pub fn into_body(self) -> B { - self.body.unwrap() + self.body } } @@ -242,7 +235,7 @@ where for (key, val) in self.head.headers.iter() { let _ = writeln!(f, " {:?}: {:?}", key, val); } - let _ = writeln!(f, " body: {:?}", self.body.as_ref().unwrap().size()); + let _ = writeln!(f, " body: {:?}", self.body.size()); res } } @@ -254,31 +247,6 @@ impl Default for Response { } } -mod fut { - use std::{ - convert::Infallible, - future::Future, - pin::Pin, - task::{Context, Poll}, - }; - - use super::*; - - // TODO: document why this is needed - impl Future for Response { - type Output = Result, Infallible>; - - fn poll(mut self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll { - Poll::Ready(Ok(Response { - head: self.head.take(), - body: self.body.take(), - error: self.error.take(), - })) - } - } -} - -/// Helper converters impl>, E: Into> From> for Response { fn from(res: Result) -> Self { match res { diff --git a/actix-http/src/response_builder.rs b/actix-http/src/response_builder.rs index f4764ff5d..6c10cacca 100644 --- a/actix-http/src/response_builder.rs +++ b/actix-http/src/response_builder.rs @@ -254,7 +254,7 @@ impl ResponseBuilder { Ok(Response { head: response, - body: Some(body), + body, error: None, }) } diff --git a/src/response/response.rs b/src/response/response.rs index a7a76e79a..ded79ca0d 100644 --- a/src/response/response.rs +++ b/src/response/response.rs @@ -2,6 +2,7 @@ use std::{ cell::{Ref, RefMut}, fmt, future::Future, + mem, pin::Pin, task::{Context, Poll}, }; @@ -11,7 +12,6 @@ use actix_http::{ http::{header::HeaderMap, StatusCode}, Extensions, Response, ResponseHead, }; -use futures_core::ready; #[cfg(feature = "cookies")] use { @@ -278,21 +278,24 @@ impl From> for Response { } } -impl Future for HttpResponse { - type Output = Result, Error>; +// Future is only implemented for Body payload type because it's the most useful for making simple +// handlers without async blocks. Making it generic over all MessageBody types requires a future +// impl on Response which would cause it's body field to be, undesirably, Option. +// +// This impl is not particularly efficient due to the Response construction and should probably +// not be invoked if performance is important. Prefer an async fn/block in such cases. +impl Future for HttpResponse { + type Output = Result, Error>; - fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + fn poll(mut self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll { if let Some(err) = self.error.take() { return Poll::Ready(Err(err)); } - let res = &mut self.res; - actix_rt::pin!(res); - - match ready!(res.poll(cx)) { - Ok(val) => Poll::Ready(Ok(val)), - Err(err) => Poll::Ready(Err(err.into())), - } + Poll::Ready(Ok(mem::replace( + &mut self.res, + Response::new(StatusCode::default()), + ))) } } diff --git a/tests/test_server.rs b/tests/test_server.rs index f25c97d99..756c180fc 100644 --- a/tests/test_server.rs +++ b/tests/test_server.rs @@ -901,7 +901,7 @@ async fn test_normalize() { let srv = actix_test::start_with(actix_test::config().h1(), || { App::new() .wrap(NormalizePath::new(TrailingSlash::Trim)) - .service(web::resource("/one").route(web::to(|| HttpResponse::Ok().finish()))) + .service(web::resource("/one").route(web::to(|| HttpResponse::Ok()))) }); let response = srv.get("/one/").send().await.unwrap();