From cd8e7293831a3dae3d2f303d8e37128fccb395ca Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Mon, 21 Mar 2022 11:19:02 +0100 Subject: [PATCH 1/2] service: Impl `Service` for `async` functions This PR just a proposal, hence being a draft, but I think its something we should at least consider for tower-service 1.0. The idea is to remove the blanket `Service` impls for `&mut S` and `Box` and replace them with an impl for `F where FnMut(Request) -> Future>`, and then remove `service_fn`. I personally think and impl for async fns is more valuable mainly to hammer home the "services are just async fns" line. I haven't yet encountered any actual need for the previous impls. `Ready` was the only thing in tower that used it and it could be easily changed to not require them. If we decide to do this I think we should consider doing the same for `Layer`, i.e. remove `impl Layer for &'a L` and make `Fn(S) -> S2` impl `Layer`. This came out of some talk in #650. --- tower-service/src/lib.rs | 38 ++++--------- tower/examples/tower-balance.rs | 4 +- tower/src/builder/mod.rs | 52 ----------------- tower/src/lib.rs | 2 +- tower/src/make/make_service/shared.rs | 5 +- tower/src/util/mod.rs | 18 +++--- tower/src/util/ready.rs | 20 +++++-- tower/src/util/service_fn.rs | 82 --------------------------- tower/tests/util/service_fn.rs | 3 +- 9 files changed, 42 insertions(+), 182 deletions(-) delete mode 100644 tower/src/util/service_fn.rs diff --git a/tower-service/src/lib.rs b/tower-service/src/lib.rs index c2f531e1a..52f561258 100644 --- a/tower-service/src/lib.rs +++ b/tower-service/src/lib.rs @@ -348,36 +348,22 @@ pub trait Service { fn call(&mut self, req: Request) -> Self::Future; } -impl<'a, S, Request> Service for &'a mut S +impl Service for F where - S: Service + 'a, + F: FnMut(Request) -> Fut, + Fut: Future>, { - type Response = S::Response; - type Error = S::Error; - type Future = S::Future; + type Response = T; + type Error = E; + type Future = Fut; - fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - (**self).poll_ready(cx) + #[inline] + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) } - fn call(&mut self, request: Request) -> S::Future { - (**self).call(request) - } -} - -impl Service for Box -where - S: Service + ?Sized, -{ - type Response = S::Response; - type Error = S::Error; - type Future = S::Future; - - fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - (**self).poll_ready(cx) - } - - fn call(&mut self, request: Request) -> S::Future { - (**self).call(request) + #[inline] + fn call(&mut self, req: Request) -> Self::Future { + self(req) } } diff --git a/tower/examples/tower-balance.rs b/tower/examples/tower-balance.rs index 998bb7c51..06777c413 100644 --- a/tower/examples/tower-balance.rs +++ b/tower/examples/tower-balance.rs @@ -119,7 +119,7 @@ fn gen_disco() -> impl Discover< .iter() .enumerate() .map(|(instance, latency)| { - let svc = tower::service_fn(move |_| { + let svc = move |_| { let start = Instant::now(); let maxms = u64::from(latency.subsec_millis()) @@ -131,7 +131,7 @@ fn gen_disco() -> impl Discover< let latency = start.elapsed(); Ok(Rsp { latency, instance }) } - }); + }; (instance, ConcurrencyLimit::new(svc, ENDPOINT_CAPACITY)) }) diff --git a/tower/src/builder/mod.rs b/tower/src/builder/mod.rs index 33684137d..65de3cdb8 100644 --- a/tower/src/builder/mod.rs +++ b/tower/src/builder/mod.rs @@ -509,58 +509,6 @@ impl ServiceBuilder { self.layer.layer(service) } - /// Wrap the async function `F` with the middleware provided by this [`ServiceBuilder`]'s - /// [`Layer`]s, returning a new [`Service`]. - /// - /// This is a convenience method which is equivalent to calling - /// [`ServiceBuilder::service`] with a [`service_fn`], like this: - /// - /// ```rust - /// # use tower::{ServiceBuilder, service_fn}; - /// # async fn handler_fn(_: ()) -> Result<(), ()> { Ok(()) } - /// # let _ = { - /// ServiceBuilder::new() - /// // ... - /// .service(service_fn(handler_fn)) - /// # }; - /// ``` - /// - /// # Example - /// - /// ```rust - /// use std::time::Duration; - /// use tower::{ServiceBuilder, ServiceExt, BoxError, service_fn}; - /// - /// # #[tokio::main] - /// # async fn main() -> Result<(), BoxError> { - /// async fn handle(request: &'static str) -> Result<&'static str, BoxError> { - /// Ok(request) - /// } - /// - /// let svc = ServiceBuilder::new() - /// .buffer(1024) - /// .timeout(Duration::from_secs(10)) - /// .service_fn(handle); - /// - /// let response = svc.oneshot("foo").await?; - /// - /// assert_eq!(response, "foo"); - /// # Ok(()) - /// # } - /// ``` - /// - /// [`Layer`]: crate::Layer - /// [`Service`]: crate::Service - /// [`service_fn`]: crate::service_fn - #[cfg(feature = "util")] - #[cfg_attr(docsrs, doc(cfg(feature = "util")))] - pub fn service_fn(self, f: F) -> L::Service - where - L: Layer>, - { - self.service(crate::util::service_fn(f)) - } - /// Check that the builder implements `Clone`. /// /// This can be useful when debugging type errors in `ServiceBuilder`s with lots of layers. diff --git a/tower/src/lib.rs b/tower/src/lib.rs index 10230b8da..0f324e8e4 100644 --- a/tower/src/lib.rs +++ b/tower/src/lib.rs @@ -220,7 +220,7 @@ pub mod layer; #[cfg(feature = "util")] #[cfg_attr(docsrs, doc(cfg(feature = "util")))] #[doc(inline)] -pub use self::util::{service_fn, ServiceExt}; +pub use self::util::ServiceExt; #[doc(inline)] pub use crate::builder::ServiceBuilder; diff --git a/tower/src/make/make_service/shared.rs b/tower/src/make/make_service/shared.rs index fd308a02a..09c422d18 100644 --- a/tower/src/make/make_service/shared.rs +++ b/tower/src/make/make_service/shared.rs @@ -106,7 +106,6 @@ opaque_future! { mod tests { use super::*; use crate::make::MakeService; - use crate::service_fn; use futures::future::poll_fn; async fn echo(req: R) -> Result { @@ -115,7 +114,7 @@ mod tests { #[tokio::test] async fn as_make_service() { - let mut shared = Shared::new(service_fn(echo::<&'static str>)); + let mut shared = Shared::new(echo::<&'static str>); poll_fn(|cx| MakeService::<(), _>::poll_ready(&mut shared, cx)) .await @@ -130,7 +129,7 @@ mod tests { #[tokio::test] async fn as_make_service_into_service() { - let shared = Shared::new(service_fn(echo::<&'static str>)); + let shared = Shared::new(echo::<&'static str>); let mut shared = MakeService::<(), _>::into_service(shared); poll_fn(|cx| Service::<()>::poll_ready(&mut shared, cx)) diff --git a/tower/src/util/mod.rs b/tower/src/util/mod.rs index dddf8ed7a..33e430ba9 100644 --- a/tower/src/util/mod.rs +++ b/tower/src/util/mod.rs @@ -16,7 +16,6 @@ mod map_future; mod oneshot; mod optional; mod ready; -mod service_fn; mod then; pub use self::{ @@ -33,7 +32,6 @@ pub use self::{ oneshot::Oneshot, optional::Optional, ready::{Ready, ReadyOneshot}, - service_fn::{service_fn, ServiceFn}, then::{Then, ThenLayer}, }; @@ -951,7 +949,7 @@ pub trait ServiceExt: tower_service::Service { /// # Example /// /// ``` - /// use tower::{Service, ServiceExt, BoxError, service_fn, util::BoxService}; + /// use tower::{Service, ServiceExt, BoxError, util::BoxService}; /// # /// # struct Request; /// # struct Response; @@ -959,9 +957,9 @@ pub trait ServiceExt: tower_service::Service { /// # fn new() -> Self { Self } /// # } /// - /// let service = service_fn(|req: Request| async { - /// Ok::<_, BoxError>(Response::new()) - /// }); + /// async fn service(req: Request) -> Result { + /// Ok(Response::new()) + /// } /// /// let service: BoxService = service /// .map_request(|req| { @@ -997,7 +995,7 @@ pub trait ServiceExt: tower_service::Service { /// # Example /// /// ``` - /// use tower::{Service, ServiceExt, BoxError, service_fn, util::BoxCloneService}; + /// use tower::{Service, ServiceExt, BoxError, util::BoxCloneService}; /// # /// # struct Request; /// # struct Response; @@ -1005,9 +1003,9 @@ pub trait ServiceExt: tower_service::Service { /// # fn new() -> Self { Self } /// # } /// - /// let service = service_fn(|req: Request| async { - /// Ok::<_, BoxError>(Response::new()) - /// }); + /// async fn service(req: Request) -> Result { + /// Ok(Response::new()) + /// } /// /// let service: BoxCloneService = service /// .map_request(|req| { diff --git a/tower/src/util/ready.rs b/tower/src/util/ready.rs index 668d50143..2bd67a437 100644 --- a/tower/src/util/ready.rs +++ b/tower/src/util/ready.rs @@ -67,7 +67,10 @@ where /// [`Ready`] values are produced by [`ServiceExt::ready`]. /// /// [`ServiceExt::ready`]: crate::util::ServiceExt::ready -pub struct Ready<'a, T, Request>(ReadyOneshot<&'a mut T, Request>); +pub struct Ready<'a, T, Request> { + inner: Option<&'a mut T>, + _p: PhantomData Request>, +} // Safety: This is safe for the same reason that the impl for ReadyOneshot is safe. impl<'a, T, Request> Unpin for Ready<'a, T, Request> {} @@ -78,7 +81,10 @@ where { #[allow(missing_docs)] pub fn new(service: &'a mut T) -> Self { - Self(ReadyOneshot::new(service)) + Self { + inner: Some(service), + _p: PhantomData, + } } } @@ -89,7 +95,13 @@ where type Output = Result<&'a mut T, T::Error>; fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - Pin::new(&mut self.0).poll(cx) + ready!(self + .inner + .as_mut() + .expect("poll after Poll::Ready") + .poll_ready(cx))?; + + Poll::Ready(Ok(self.inner.take().expect("poll after Poll::Ready"))) } } @@ -98,6 +110,6 @@ where T: fmt::Debug, { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_tuple("Ready").field(&self.0).finish() + f.debug_tuple("Ready").field(&self.inner).finish() } } diff --git a/tower/src/util/service_fn.rs b/tower/src/util/service_fn.rs deleted file mode 100644 index d6e6be878..000000000 --- a/tower/src/util/service_fn.rs +++ /dev/null @@ -1,82 +0,0 @@ -use std::fmt; -use std::future::Future; -use std::task::{Context, Poll}; -use tower_service::Service; - -/// Returns a new [`ServiceFn`] with the given closure. -/// -/// This lets you build a [`Service`] from an async function that returns a [`Result`]. -/// -/// # Example -/// -/// ``` -/// use tower::{service_fn, Service, ServiceExt, BoxError}; -/// # struct Request; -/// # impl Request { -/// # fn new() -> Self { Self } -/// # } -/// # struct Response(&'static str); -/// # impl Response { -/// # fn new(body: &'static str) -> Self { -/// # Self(body) -/// # } -/// # fn into_body(self) -> &'static str { self.0 } -/// # } -/// -/// # #[tokio::main] -/// # async fn main() -> Result<(), BoxError> { -/// async fn handle(request: Request) -> Result { -/// let response = Response::new("Hello, World!"); -/// Ok(response) -/// } -/// -/// let mut service = service_fn(handle); -/// -/// let response = service -/// .ready() -/// .await? -/// .call(Request::new()) -/// .await?; -/// -/// assert_eq!("Hello, World!", response.into_body()); -/// # -/// # Ok(()) -/// # } -/// ``` -pub fn service_fn(f: T) -> ServiceFn { - ServiceFn { f } -} - -/// A [`Service`] implemented by a closure. -/// -/// See [`service_fn`] for more details. -#[derive(Copy, Clone)] -pub struct ServiceFn { - f: T, -} - -impl fmt::Debug for ServiceFn { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("ServiceFn") - .field("f", &format_args!("{}", std::any::type_name::())) - .finish() - } -} - -impl Service for ServiceFn -where - T: FnMut(Request) -> F, - F: Future>, -{ - type Response = R; - type Error = E; - type Future = F; - - fn poll_ready(&mut self, _: &mut Context<'_>) -> Poll> { - Ok(()).into() - } - - fn call(&mut self, req: Request) -> Self::Future { - (self.f)(req) - } -} diff --git a/tower/tests/util/service_fn.rs b/tower/tests/util/service_fn.rs index ac6bf06f3..488046283 100644 --- a/tower/tests/util/service_fn.rs +++ b/tower/tests/util/service_fn.rs @@ -1,12 +1,11 @@ use futures_util::future::ready; -use tower::util::service_fn; use tower_service::Service; #[tokio::test(flavor = "current_thread")] async fn simple() { let _t = super::support::trace_init(); - let mut add_one = service_fn(|req| ready(Ok::<_, ()>(req + 1))); + let mut add_one = |req| ready(Ok::<_, ()>(req + 1)); let answer = add_one.call(1).await.unwrap(); assert_eq!(answer, 2); } From f3b15c100cfdf5b244a91f4234e216afe49b6c03 Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Tue, 22 Mar 2022 09:08:11 +0100 Subject: [PATCH 2/2] Add `ByRef` --- tower/src/util/by_ref.rs | 29 +++++++++++++++++++++++++++++ tower/src/util/mod.rs | 10 ++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tower/src/util/by_ref.rs diff --git a/tower/src/util/by_ref.rs b/tower/src/util/by_ref.rs new file mode 100644 index 000000000..3021fba12 --- /dev/null +++ b/tower/src/util/by_ref.rs @@ -0,0 +1,29 @@ +use std::task::{Context, Poll}; + +use tower_service::Service; + +/// TODO +#[derive(Debug)] +pub struct ByRef<'a, S>(&'a mut S); + +impl<'a, S> ByRef<'a, S> { + pub(crate) fn new(service: &'a mut S) -> Self { + Self(service) + } +} + +impl<'a, S, Request> Service for ByRef<'a, S> where S: Service { + type Response = S::Response; + type Error = S::Error; + type Future = S::Future; + + #[inline] + fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + (*self.0).poll_ready(cx) + } + + #[inline] + fn call(&mut self, req: Request) -> Self::Future { + (*self.0).call(req) + } +} diff --git a/tower/src/util/mod.rs b/tower/src/util/mod.rs index 33e430ba9..06f040ba4 100644 --- a/tower/src/util/mod.rs +++ b/tower/src/util/mod.rs @@ -12,6 +12,7 @@ mod map_request; mod map_response; mod map_result; +mod by_ref; mod map_future; mod oneshot; mod optional; @@ -22,6 +23,7 @@ pub use self::{ and_then::{AndThen, AndThenLayer}, boxed::{BoxLayer, BoxService, UnsyncBoxService}, boxed_clone::BoxCloneService, + by_ref::ByRef, either::Either, future_service::{future_service, FutureService}, map_err::{MapErr, MapErrLayer}, @@ -1034,6 +1036,14 @@ pub trait ServiceExt: tower_service::Service { { BoxCloneService::new(self) } + + /// TODO + fn by_ref(&mut self) -> ByRef<'_, Self> + where + Self: Sized, + { + ByRef::new(self) + } } impl ServiceExt for T where T: tower_service::Service {}