Skip to content

Commit

Permalink
util: add FutureService::new, with relaxed bounds
Browse files Browse the repository at this point in the history
There are a couple issues with the current implementation of
`FutureService`.

The first, and less important, is a minor usability
issue: there's no `FutureService::new`, just a free function that
returns `FutureService`. While the free function is nice in some cases,
it means that if a user *is* naming the type someplace, they need to
import `tower::util::future_service` *and* `tower::util::FutureService`,
which is slightly annoying. Also, it just kind of violates the common
assumption that most publicly constructable types have a `new`,
requiring a look at the docs.

The second, more significant issue is that the `future_service` function
places a `Service` bound on the future's output type. While this is of
course necessary for the *`Service` impl* on `FutureService`, it's not
required to construct a `FutureService`. Of course, you generally don't
want to construct a `FutureService` that *won't* implement service.
However, the bound also means that additional generic parameters are now
required at the site where the `FutureService` is constructed. In
particular, the caller must now either know the request type, or be
generic over one.

In practice, when other middleware returns or constructs a
`FutureService`, this essentially means that it's necessary to add a
`PhantomData` for the request type parameter. This complicates code, and
perhaps more importantly, increases compile times, especially with
deeply-ensted middleware stacks.

As an example of the downside of aggressive bounds at the constructor,
it's worth noting that the implementation of `FutureService` currently
in `tower` is based directly on a similar implementation in
`linkerd2-proxy`. Except for the difference of whether or not the
constructor has a `Service` bound on the future's output, the two
implementations are very similar, almost identical. This gist shows some
of the change necessary to replace our otherwise identical
implementation with the `tower` version that bounds the `Service` type
at construction-time:

https://gist.github.com/hawkw/a6b07f9f4a8bce0c4b61036ed94114db

This PR solves these issues by adding a `FutureService::new` constructor
that does not introduce the `Service` bound. I didn't change the
`future_service` function: I don't *think* removing bounds is a breaking
change, but it is a modification to a publicly exposed function's type
signature, so I'm a little leery about it. Also, I thought that the more
aggressive bounding at construction-time might still be useful in
simpler use-cases where the `FutureService` is not part of a more
complex middleware stack, and that the free fn might be more likely to
be used in those cases anyway.

cc @davidpdrsn
  • Loading branch information
hawkw committed Jan 12, 2021
1 parent b4169c7 commit de683e5
Showing 1 changed file with 51 additions and 3 deletions.
54 changes: 51 additions & 3 deletions tower/src/util/future_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ where
F: Future<Output = Result<S, E>> + Unpin,
S: Service<R, Error = E>,
{
FutureService {
state: State::Future(future),
}
FutureService::new(future)
}

/// A type that implements [`Service`] for a [`Future`] that produces a [`Service`].
Expand All @@ -63,6 +61,56 @@ pub struct FutureService<F, S> {
state: State<F, S>,
}

impl<F, S> FutureService<F, S> {
/// Returns a new [`FutureService`] for the given future.
///
/// A [`FutureService`] allows you to treat a future that resolves to a service as a service. This
/// can be useful for services that are created asynchronously.
///
/// # Example
/// ```
/// use tower::{service_fn, Service, ServiceExt};
/// use tower::util::FutureService;
/// use std::convert::Infallible;
///
/// # fn main() {
/// # async {
/// // A future which outputs a type implementing `Service`.
/// let future_of_a_service = async {
/// let svc = service_fn(|_req: ()| async { Ok::<_, Infallible>("ok") });
/// Ok::<_, Infallible>(svc)
/// };
///
/// // Wrap the future with a `FutureService`, allowing it to be used
/// // as a service without awaiting the future's completion:
/// let mut svc = FutureService::new(Box::pin(future_of_a_service));
///
/// // Now, when we wait for the service to become ready, it will
/// // drive the future to completion internally.
/// let svc = svc.ready_and().await.unwrap();
/// let res = svc.call(()).await.unwrap();
/// # };
/// # }
/// ```
///
/// # Regarding the [`Unpin`] bound
///
/// The [`Unpin`] bound on `F` is necessary because the future will be polled in
/// [`Service::poll_ready`] which doesn't have a pinned receiver (it takes `&mut self` and not `self:
/// Pin<&mut Self>`). So we cannot put the future into a `Pin` without requiring `Unpin`.
///
/// This will most likely come up if you're calling `future_service` with an async block. In that
/// case you can use `Box::pin(async { ... })` as shown in the example.
pub fn new(future: F) -> Self
where
F: Future + Unpin,
{
Self {
state: State::Future(future),
}
}
}

impl<F, S> fmt::Debug for FutureService<F, S>
where
S: fmt::Debug,
Expand Down

0 comments on commit de683e5

Please sign in to comment.