Add Future-like combinators #396

hlb8122 · 2019-12-17T16:18:06Z

Motivation
It is often the case that one wishes to take a service and modify it's output or input in some way without changing the behavior of poll_ready(). For example, turning a service returning Result<(Parts, Body), _> into a service returning Result<Request<Body>, _> or modifying the error type.

Currently, there is no ergonomic way to apply such changes without a fair amount of boilerplate.

Proposal
Extend ServiceExt trait to add various methods mirroring the combinators from TryFutureExt and various services and layers to accommodate this.

Requirements
Because combinators could be heavily chained together it's probably wise to trim all the fat off here and if possible make it zero-cost when compared to simply applying the combinators inside of the inner service's call.

Result
Having such methods should allow a more expressive algebra of tools to construct and combine services.

Current Implementation
At the time of writing I've introduced four combinators:

    fn map_ok<F, Response>(self, f: F) -> MapOk<Self, F>
    where
        Self: Sized,
        F: FnOnce(Self::Response) -> Response + Clone,
    {
        MapOk::new(self, f)
    }

    fn map_err<F, Error>(self, f: F) -> MapErr<Self, F>
    where
        Self: Sized,
        F: FnOnce(Self::Error) -> Error + Clone,
    {
        MapErr::new(self, f)
    }

    fn with<F, NewRequest>(self, f: F) -> With<Self, F>
    where
        Self: Sized,
        F: FnOnce(NewRequest) -> Request + Clone,
    {
        With::new(self, f)
    }

    fn try_with<F, NewRequest>(self, f: F) -> With<Self, F>
    where
        Self: Sized,
        F: FnOnce(NewRequest) -> Result<Request, Self::Error> + Clone,
    {
        TryWith::new(self, f)
    }

and an associated Service and Layer for each.

TODO

Which trait constraints are minimally required on the closures to ensure an ergonomic API without any excess overhead?
Documentation
More combinators such as and_then

tower-util/src/combinators/map_err.rs

tower-util/src/combinators/map_ok.rs

tower-util/src/combinators/with.rs

tower-util/src/lib.rs

tower-util/src/combinators/with.rs

LucioFranco · 2019-12-19T16:20:27Z

This looks like a fantastic start! I would like to review this once we get the 0.3 release out since we can always add more items in follow up patches.

hlb8122 · 2020-06-26T12:27:49Z

Has there been any decisions made with respect to this? I noticed map_request has been implemented in #435.

hawkw

Thanks for working on this, I'd definitely like to get this in!

I had some suggestions, including a few style nits & API design questions to think about.

Also, I think a lot of this code has moved around on master --- would you mind rebasing and moving the new code to track that? I don't think there are any other differences that should effect this PR besides code moving around.

tower-util/src/combinators/map_err.rs

hawkw · 2020-06-26T16:30:05Z

tower-util/src/combinators/map_err.rs

+    F: FnOnce(S::Error) -> Error,
+    F: Clone,


Using FnOnce + Clone here seemed a little weird at first, but then I realized that the requirement that the function be moved into the MapErr future and cloned in Layer::layer kind of drives this. So if we're going to have to require Clone, it makes sense to use FnOnce.

hawkw · 2020-06-26T16:38:13Z

tower-util/src/combinators/with.rs

+use tower_layer::Layer;
+use tower_service::Service;
+
+pub struct With<S, F> {


tower-http contains a tower-request-modifier crate that does a bunch of HTTP-specific request mutations...I wonder if that could be reimplemented on top of this API, eventually? It would be nice to keep that in mind and make sure this is possible.

I'll give it a fork and try it out when I get some spare time.

tower-rs/tower-http#31

I departed from the previous API by a fair bit but I think it turned out ok.

tower-util/src/combinators/with.rs

tower-util/src/lib.rs

hawkw

This looks really good to me!

I noticed a couple things that would be good to address:

CI will warn us unless we add doc comments to all the new public types introduced here --- let's do that. Since the main way users will interact with these types will be via the combinators, it's okay if the types' docs are pretty minimal, as long as they link to the combinator that creates them. This is essentially what std's docs do for iterator combinators etc.
Looks like the examples don't actually compile on CI --- let's fix that, please!

Once those issues are addressed, I would be happy to merge this, unless @LucioFranco or @jonhoo have any thoughts on naming.

tower/src/util/combinators/map_err.rs

tower/src/util/combinators/map_ok.rs

tower/src/util/mod.rs

hlb8122 · 2020-06-30T04:20:18Z

Quick note about the documentation: I've used intra-doc links here which are only supported on nightly (and the docs.rs compiler).

The reason for this is that the of #[doc(inline)] on ServiceExt in lib.rs breaks the relative links.

The same problem exists for the CallAll link in the documentation of ServiceExt::call_all method from the /doc/tower/trait.ServiceExt.html page (rather than /doc/tower/util/trait.ServiceExt.html)

LucioFranco

Super sorry for the delay on reviewing this!!!!!

Overall, looks fantastic, I have a couple comments inline. Let me know if you have any questions. The Map layer that exists already is probably a good reference point and we should try and match that.

LucioFranco · 2020-07-01T19:04:30Z

tower/src/util/combinators/map_err.rs

+impl<S, F, Request, Error> Service<Request> for MapErr<S, F>
+where
+    S: Service<Request>,
+    F: FnOnce(S::Error) -> Error + Clone,


Why clone? Why not Fn(S::Error) -> Error? I believe Fn is semantically similar to FnOnce + Clone while being less restrictive.

@LucioFranco I don't think that's the case. Fn can be called with an & reference. However, since the function is called after the inner service's future, the function needs to be moved into the future that the MapErr service returns. Therefore, it cannot be borrowed without constraining the future's lifetime --- so it must be Cloned. And if it has to be Clone, it may as well be FnOnce, since each clone will only be called a single time.

If we wanted to use Fn, we would need to wrap it in an Arc internally, and clone the Arc into each future. That would also work, but in some cases (such as function pointers), the Arc isn't needed, since little-f fn can be freely cloned already.

@hawkw is right, we should then change the bounds on the map combinators as well.

LucioFranco · 2020-07-01T19:07:02Z

tower/src/util/combinators/map_err.rs

+{
+    type Response = S::Response;
+    type Error = Error;
+    type Future = MapErrFut<S::Future, F>;


I've been using MapErrFuture instead of just Fut. See the map combinator.

tower/src/util/combinators/map_err.rs

tower/src/util/combinators/map_ok.rs

LucioFranco · 2020-07-01T19:13:47Z

tower/src/util/combinators/map_ok.rs

+{
+    type Response = Response;
+    type Error = S::Error;
+    type Future = MapOkFut<S::Future, F>;


Same as above.

tower/src/util/combinators/try_with.rs

LucioFranco · 2020-07-01T19:16:08Z

tower/src/util/combinators/try_with.rs

+use tower_layer::Layer;
+use tower_service::Service;
+
+/// Service returned by the [`try_with`] combinator.


can we get a bit more in the docs here? The name isn't clear enough that we probably want to add some more docs.

We can also link to the docs in the extension trait.

This should link to the combinator that creates it, which is the authoritative source for the docs. It's also what users will probably see first. Since the combinator docs are linked, I don't think the type really needs any additional documentation --- it would just add the burden of having to maintain two separate versions of essentially the same documentation, keep them in sync, and so on.

This is basically what std does for iterator combinators etc.

tower/src/util/combinators/try_with.rs

LucioFranco · 2020-07-01T19:16:46Z

tower/src/util/combinators/with.rs

+use tower_layer::Layer;
+use tower_service::Service;
+
+/// Service returned by the [`with`] combinator.


Same as above.

tower/src/util/combinators/with.rs

LucioFranco

LGTM!

LucioFranco · 2020-07-15T18:45:09Z

Thanks @hlb8122 a ton!

tower/src/util/map_err.rs

tower/src/util/map_ok.rs

tower/src/util/mod.rs

hawkw · 2020-07-15T20:13:54Z

tower/src/util/mod.rs

+    /// Composes a function *in front of* the service.
+    ///
+    /// This adapter produces a new service that passes each value through the
+    /// given function `f` before sending it to `self`.


Perhaps this should reference the futures::SinkExt::with method as a comparison? Not a blocker.

hawkw · 2020-07-15T20:14:17Z

tower/src/util/mod.rs

+    ///
+    /// This adapter produces a new service that passes each value through the
+    /// given function `f` before sending it to `self`.
+    ///


Again, maybe worth linking futures::SinkExt::try_with here? Not a big deal.

tower/src/util/try_with.rs

tower-util/src/lib.rs

hawkw

This looks great to me --- only a handful of docs issues.

I'm not sure if the links without a reference work correctly or not. It might be worth running cargo docs --open -p tower and making sure that all the links are rendered correctly and don't 404 when clicked on.

If I'm wrong, and RustDoc can handle this correctly, please disregard my comments.

tower/src/util/try_with.rs

tower/src/util/with.rs

hlb8122 changed the title ~~(WIP) Add rudimentary combinators~~ (WIP) Add Future-like combinators Dec 17, 2019

hlb8122 commented Dec 17, 2019

View reviewed changes

tower-util/src/combinators/map_err.rs Outdated Show resolved Hide resolved

hlb8122 commented Dec 17, 2019

View reviewed changes

tower-util/src/combinators/map_ok.rs Outdated Show resolved Hide resolved

hlb8122 commented Dec 17, 2019

View reviewed changes

tower-util/src/combinators/with.rs Outdated Show resolved Hide resolved

hlb8122 commented Dec 17, 2019

View reviewed changes

tower-util/src/lib.rs Outdated Show resolved Hide resolved

hlb8122 commented Dec 18, 2019

View reviewed changes

tower-util/src/combinators/with.rs Outdated Show resolved Hide resolved

jonhoo requested a review from LucioFranco March 31, 2020 22:21

jonhoo mentioned this pull request Mar 31, 2020

Implement mapping combinators in tower #272

Closed

jonhoo assigned LucioFranco Mar 31, 2020

jonhoo added A-util Area: The tower "util" module C-enhancement Category: A PR with an enhancement or a proposed on in an issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2020

hlb8122 marked this pull request as draft April 15, 2020 19:48

hdevalence mentioned this pull request Jun 3, 2020

start work of porting to next version of tower with map_err ZcashFoundation/zebra#415

Closed

hlb8122 changed the title ~~(WIP) Add Future-like combinators~~ Add Future-like combinators Jun 26, 2020

hawkw reviewed Jun 26, 2020

View reviewed changes

hlbarber added 7 commits June 26, 2020 23:41

Add rudimentary combinators

75f3521

Add doc tests for map_ok, map_err, and with

ad77280

Remove redundant tests and cargo fmt

3c66ab2

Fix typo in documentation

1efdc6c

Move combinators

3a57a67

Refactor phantom data fields

0a0358b

Simplify PhantomData constructors

c11f11b

hlb8122 force-pushed the service-combinators branch from 320b2f5 to c11f11b Compare June 26, 2020 23:30

hlbarber added 4 commits June 27, 2020 01:19

cargo fmt

c4a3134

Remove useless constraints

97642d3

Add missing debug

9acdcb8

Add better documentation for map_ok and map_err

2ae116c

hlbarber added 5 commits June 28, 2020 14:49

Add documentation to "with"

3c06dd7

Add TryWith and TryWithLayer

45fb7dd

Add try_with combinator to ServiceExt

4d028e6

Remove "unreachable_pub" warning from combinators

b082b12

Add try_with documentation

aa3e593

hawkw reviewed Jun 29, 2020

View reviewed changes

hlbarber added 4 commits June 30, 2020 02:39

Improve documentation

e443087

Switch to intra-doc links

35c1d6b

Improve and fix documentation tests

e8ef586

cargo fmt

aa7a721

hlb8122 marked this pull request as ready for review June 30, 2020 04:32

Improve consistency across trait bound layout

1148f52

hlb8122 mentioned this pull request Jun 30, 2020

Refactor using tower combinators tower-rs/tower-http#31

Merged

LucioFranco requested changes Jul 1, 2020

View reviewed changes

hlbarber added 2 commits July 15, 2020 03:31

Relax bounds on combinator layers

2a245fb

NewRequest -> R1, OldRequest -> R2 for combinator generics

e1e7a68

hlb8122 force-pushed the service-combinators branch from 0c18eb9 to e1e7a68 Compare July 15, 2020 02:40

Move combinators from /util/combinators/* to /util/*

f58f90f

LucioFranco approved these changes Jul 15, 2020

View reviewed changes

hawkw reviewed Jul 15, 2020

View reviewed changes

hawkw approved these changes Jul 15, 2020

View reviewed changes

tower/src/util/try_with.rs Show resolved Hide resolved

tower/src/util/with.rs Show resolved Hide resolved

tower/src/util/with.rs Show resolved Hide resolved

tower/src/util/with.rs Show resolved Hide resolved

LucioFranco merged commit ab7518e into tower-rs:master Jul 17, 2020

hdevalence mentioned this pull request Sep 2, 2020

0.3.2 release with service combinators #468

Closed

hawkw mentioned this pull request Dec 28, 2020

util: unify and rename combinators #499

Merged

dependabot bot mentioned this pull request Mar 11, 2021

chore(deps): bump tower-test from 0.3.0 to 0.4.0 CleverCloud/vector#6

Closed

hlb8122 deleted the service-combinators branch January 15, 2022 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Future-like combinators #396

Add Future-like combinators #396

hlb8122 commented Dec 17, 2019 •

edited

Loading

LucioFranco commented Dec 19, 2019

hlb8122 commented Jun 26, 2020

hawkw left a comment

hawkw Jun 26, 2020

hawkw Jun 26, 2020

hlb8122 Jun 28, 2020 •

edited

Loading

hlb8122 Jun 30, 2020

hawkw left a comment

hlb8122 commented Jun 30, 2020

LucioFranco left a comment

LucioFranco Jul 1, 2020

hawkw Jul 1, 2020

LucioFranco Jul 1, 2020

LucioFranco Jul 1, 2020

LucioFranco Jul 1, 2020

LucioFranco Jul 1, 2020

LucioFranco Jul 1, 2020

hawkw Jul 1, 2020

LucioFranco Jul 1, 2020

LucioFranco left a comment

LucioFranco commented Jul 15, 2020

hawkw Jul 15, 2020

hawkw Jul 15, 2020

hawkw left a comment

Add Future-like combinators #396

Add Future-like combinators #396

Conversation

hlb8122 commented Dec 17, 2019 • edited Loading

LucioFranco commented Dec 19, 2019

hlb8122 commented Jun 26, 2020

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlb8122 Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hlb8122 commented Jun 30, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hlb8122 commented Dec 17, 2019 •

edited

Loading

hlb8122 Jun 28, 2020 •

edited

Loading