Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: add FutureService::new, with relaxed bounds #523

Merged
merged 5 commits into from
Jan 13, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jan 12, 2021

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

@hawkw hawkw requested review from olix0r and LucioFranco January 12, 2021 00:07
@hawkw hawkw self-assigned this Jan 12, 2021
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
@hawkw hawkw force-pushed the eliza/new-future-service branch from de683e5 to e879f76 Compare January 12, 2021 00:11
@davidpdrsn
Copy link
Member

Makes sense 👍

I also prefer constructors over free standing functions mainly because its easier to be consistent with.

I also think removing the generics bounds on future_service should be fine but I understand your concern. It is a bit odd that the functions don't have the same bounds though 🤔

@olix0r
Copy link
Collaborator

olix0r commented Jan 12, 2021

Should we add a deprecation on the free function and/or hide its docs?

@hawkw
Copy link
Member Author

hawkw commented Jan 12, 2021

Should we add a deprecation on the free function and/or hide its docs?

🤷‍♀️ I don't really think it's necessary to deprecate it, and the free fn APIs can be more convenient in some cases (e.g. service_fn, layer_fn etc). I just think any types with a free fn constructor should also have a new.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/new-future-service branch from 6ddf09a to 5ee4fd3 Compare January 12, 2021 18:02
This reverts commit 5ee4fd3. This
actually *is* breaking --- it would mean removing the `R` type parameter
for the request type on the function. This changes the function
definition, which might break uses of it.
@hawkw
Copy link
Member Author

hawkw commented Jan 12, 2021

I also think removing the generics bounds on future_service should be fine but I understand your concern. It is a bit odd that the functions don't have the same bounds though thinking

After thinking through it, i think this actually is breaking --- it would mean removing the R type parameter
for the request type on the function. This changes the function
definition, which might break uses of it.

@hawkw hawkw merged commit 359db0e into master Jan 13, 2021
@hawkw hawkw deleted the eliza/new-future-service branch January 13, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants