decide the future of tower::filter

[hawkw]

Motivation

It was pointed out that there is currently some overlap between the try_with Service combinator and tower::filter middleware (see #499 (comment)). try_with synchronously maps from a Request -> Result<DifferentRequest, Error>, while tower::filter asynchronously maps from a &Request to a Result<(), Error>. The key differences are:

• try_with takes a request by value, and allows the predicate to return a different request value
• try_with also permits changing the type of the request
• try_with is synchronous, while tower::filter is asynchronous
• tower::filter has a Predicate trait, which can be implemented by more than just functions. For example, a struct with a HashSet could implement Predicate by failing requests that match the values in the hashset.

It definitely seems like there's demand for both synchronous and asynchronous request filtering. However, the APIs we have currently differ pretty significantly. It would be nice to make them more consistent with each other.

As an aside, tower::filter does not seem all that widely used.

Meanwhile, linkerd2-proxy defines its own RequestFilter middleware, using a predicate trait that's essentially in between tower::filter and ServiceExt::try_with:

• it's synchronous, like try_with
• it allows modifying the type of the request, like try_with
• it uses a trait for predicates, rather than a Fn, like tower::filter
• it uses a similar naming scheme to tower::filter ("filtering" rather than "with"/"map")

Proposal

I think we should try to unify these APIs into something with a more consistent interface. I suggest the following:

• continue using the "Filter"/"Predicate" terminology rather than the function composition terminology ("with"/"map")
• take Request types by value rather than by reference, and allow returning a new request type
• continue providing traits that can be implemented by things other than functions
• provide both sync and async filtering APIs

It's theoretically possible to have one API for both sync and async filtering, but this would require a trait like IntoFuture that can be implemented by both Future<Output = Result<T, E> and Result<T, E>. A predicate trait could return something implementing that trait, and Result could have an impl that turns itself into a ready future. This would let us have one API for both sync and async predicates. However, it introduces some complexity in the type signatures, and requires a slight added overhead (polling the immediately ready future when the predicate is synchronous). The added complexity of using an IntoFuture-like trait might be less of an issue when the standard library stabilizes IntoFuture...

On the other hand, we could have separate sync and async filter APIs. The main disadvantage of that approach is that we'd then need to bikeshed a consistent naming scheme that makes both the distinction and the similarity similar. Historically, we've not been great at naming things efficiently ;)

cc @LucioFranco @seanmonstar @olix0r @hlb8122 --- what do you all think?

https://github.com/linkerd/linkerd2-proxy/blob/main/linkerd/stack/src/request_filter.rs

[hawkw]

It's theoretically possible to have one API for both sync and async filtering, but this would require a trait like IntoFuture that can be implemented by both Future<Output = Result<T, E> and Result<T, E>. A predicate trait could return something implementing that trait, and Result could have an impl that turns itself into a ready future. This would let us have one API for both sync and async predicates. However, it introduces some complexity in the type signatures, and requires a slight added overhead (polling the immediately ready future when the predicate is synchronous). The added complexity of using an IntoFuture-like trait might be less of an issue when the standard library stabilizes IntoFuture...

Actually, after thinking about it, this is only possible if std stabilizes an IntoFuture trait with an impl for Result --- otherwise, it's not coherent to have a blanket impl of a trait for Result and F: Future outside of std.

We could have a wrapper fn for turning an async predicate into a sync predicate, but that seems like "just have two separate APIs" but with more steps...

[LucioFranco]

Doesn't async move {} solve most of the main issues around easily making a sync fn into an async one? While its a bit more painful its not that bad.

[hawkw]

Doesn't async move {} solve most of the main issues around easily making a sync fn into an async one? While its a bit more painful its not that bad.

Yes, it's always possible to satisfy an async interface with a purely synchronous function using a future that doesn't await, and it's never possible to satisfy a purely synchronous interface with an async function.

However, that solution introduces some additional complexity (and overhead) in the case of tower::filter. The asynchronous filter has to clone the inner service into the future, because the async filtering behavior must run as part of the future, and we don't know whether the inner service will be called until the async filtering logic executes. A synchronous filter wouldn't need the Clone bound on the inner service type, which is significant. Also, it might be a bit more efficient, especially when rejecting requests.

That's why I think it's better to have two interfaces, so that synchronous filtering doesn't need to introduce a Clone bound.

[LucioFranco]

Yeah, I agree, I wonder if we could use a trait maybe to get around it? but that sounds hard. We will just have to bike shed the names :(