* tower: prepare to release 0.4.1
This branch updates the tower-layer dependency to 0.3.1 and prepares a
new release of `tower`. This should fix the broken re-exports of
`layer_fn` and get us a successful docs.rs build.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch updates the changelogs and version numbers for Tower 0.4.
* update changelog for 0.4
* add changelog blurb
* update version to 0.4
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
I think `Steer` is looking pretty useful. I have use case where want
some requests (such as `GET /metrics`) to go to one service and other
requests to some other service. I imagine this is quite a common use
case for `Steer` so I figure the example in the docs would be more
helpful if it showed how to accomplish something like that.
In general I think tower's docs could be better at explaining how to
combine all the different pieces to solve real problems. Hopefully this
might help a bit wrt `Steer`.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
* make all combinator futures opaque
For stability reasons, we probably don't want to expose future
combinator types from the `futures_util` crate in public APIs. If we
were to change which combinators are used to implement these futures, or
if `futures_util` made breaking changes, this could cause API breakage.
This branch wraps all publicly exposed `futures_util` types in the
`opaque_future!` macro added in #508, to wrap them in newtypes that hide
their internal details. This way, we can change these futures' internals
whenever we like.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation
It was pointed out that there is currently some overlap between the
`try_with` `Service` combinator and `tower::filter` middleware (see https://github.com/tower-rs/tower/pull/499#discussion_r549522471 ).
`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][1].
Meanwhile, `linkerd2-proxy` defines its own `RequestFilter` middleware,
using a [predicate trait][2] 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").
[1]: https://github.com/search?l=&p=1&q=%22tower%3A%3Afilter%22+extension%3Ars&ref=advsearch&type=Code
[2]: 24bee8cbc5/linkerd/stack/src/request_filter.rs (L8-L12)
## Solution
This branch rewrites `tower::filter` to make the following changes:
* Predicates are synchronous by default. A separate `AsyncFilter` type
and an `AsyncPredicate` trait are available for predicates returning
futures.
* Predicates may now return a new `Request` type, allowing `Filter` and
`AsyncFilter` to subsume `try_map_request`.
* Predicates may now return any error type, and errors are now converted
into `BoxError`s.
Closes#502
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This branch makes the following changes:
* New `lib.rs` docs for `tower`, which should hopefully provide a
better explanation of Tower's core abstractions & their
relationships
* Nicer docs for `ServiceBuilder`
* Added `#[doc(cfg(...))]` attributes for feature flagged APIs
* Example improvements
* Fixing a bunch of broken intra-rustdoc links
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Currently, `ServiceExt` and `ServiceBuilder` provide combinators for
mapping successful responses to other responses, and mapping errors to
other errors, but don't provide a way to map between `Ok` and `Err`
results.
For completeness, this branch adds a new `then` combinator, which takes
a function from `Result` to `Result` and applies it when the service's
future completes. This can be used for recovering from some errors or
for rejecting some `Ok` responses. It can also be used for behaviors
that should be run when a service's future completes regardless of
whether it completed successfully or not.
Depends on #499
Currently, the `ServiceExt` trait has `with` and `try_with` methods for
composing a `Service` with functions that modify the request type, and
`map_err` and `map_ok` methods for composing a service with functions
that modify the response type. Meanwhile, `ServiceBuilder` has
`map_request` for composing a service with a request mapping function,
and `map_response` for composing a service with a response-mapping
function. These combinators are very similar in purpose, but are
implemented using different middleware types that essentially duplicate
the same behavior.
Before releasing 0.4, we should probably unify these APIs. In
particular, it would be good to de-duplicate the middleware service
types, and to unify the naming.
This commit makes the following changes:
- Rename the `ServiceExt::with` and `ServiceExt::try_with` combinators
to `map_request` and `try_map_request`
- Rename the `ServiceExt::map_ok` combinator to `map_response`
- Unify the `ServiceBuilder::map_request` and `ServiceExt::map_request`
combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_response` and
`ServiceExt::map_response` combinators to use the same `Service` type
- Unify the `ServiceBuilder::map_err` and `ServiceExt::map_err`
combinators to use the same `Service` type
- Only take `FnOnce + Clone` when in response/err combinators, which
require cloning into the future type. `MapRequest` and `TryMapRequest`
now take `FnMut(Request)`s and don't clone them every time they're
called
- Reexport future types for combinators where it makes sense.
- Add a `try_map_request` method to `ServiceBuilder`
Closes#498
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Resolves#253
Adds `MakeService::into_service` and `MakeService::as_service` which
converts `MakeService`s into `Service`s. `into_service` consumes `self`
and `as_service` borrows `self` mutably.
This change adds `BoxService::layer` and `UnsyncBoxService::layer`
helpers to provide a convenience helper for boxing services in layered
stacks.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Resolves#264
This branch adds `FutureService` to `tower::util`, which implements
`Service` for `Future<Output = impl Service>`. Before the future has
completed, `poll_ready` on `FutureService` will drive it to completion,
returning `Ready` only once the future has completed. Subsequent calls
will poll the service returned by the future.
See https://github.com/tower-rs/tower/issues/264#issuecomment-751400523
Based on
1be301f2b0/linkerd/stack/src/future_service.rs
Co-authored-by: Daiki Mizukami <tesaguriguma@gmail.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Resolves#267
I went with `layer_fn` over `layer::from_fn` because changing
`service_fn` would be a breaking change. But I don't mind changing it if
you think thats more appropriate 😊
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
It's generally the case that layers need to be shareable. There's no
reason that `BufferLayer` should not implement both `Clone` and `Copy`.
This change adds manual `Clone` and `Copy` implementations for
`BufferLayer`.
`MakeBalance`'s use of `SmallRng` is problematic: since it clones the
`SmallRng` between `Balance` instances, each instance will have the same
state and produce an identical sequence of values. This probably isn't
_dangerous_, though it is certainly unexpected.
This change removes the `MakeBalance::from_rng` and
`MakeBalanceLayer::from_rng` helpers. The `MakeBalance` service now uses
the default RNG via `Balance::new`. `Balance::new` now creates its
`SmallRng` from the `thread_rng` instead of the default entropy source,
as the default entropy source may use the slower `getrandom`. From the
[`rand` docs][from_entropy]:
> In case the overhead of using getrandom to seed many PRNGs is an
> issue, one may prefer to seed from a local PRNG, e.g.
> from_rng(thread_rng()).unwrap().
Finally, this change updates the balnancer to the most recent version of
`rand`, v0.8.0.
[from_entropy]: https://docs.rs/rand/0.8.0/rand/trait.SeedableRng.html#method.from_entropy
This branch updates Tower to depend on Tokio v1.0. In particular, the
following changes were necessary:
* `tokio::sync::Semaphore` now has a `close` operation, so permit
acquisition is fallible. Our uses of the semaphore are updated to
handle this. Also, this allows removing the janky homemade
implementation of closing semaphores by adding a big pile of
permits!
* `tokio::sync`'s channels are no longer `Stream`s. This necessitated a
few changes:
- Replacing a few explicit `poll_next` calls with `poll_recv`
- Updating some tests that used `mpsc::Receiver` as a `Stream` to add
a wrapper type that makes it a `Stream`
- Updating `CallAll`'s examples (I changed it to just use a
`futures::channel` MPSC)
* `tokio::time::Sleep` is no longer `Unpin`. Therefore, the rate-limit
`Service` needs to `Box::pin` it. To avoid the overhead of
allocating/deallocating `Box`es every time the rate limit is
exhausted, I moved the `Sleep` out of the `State` enum and onto the
`Service` struct, and changed the code to `reset` it every time the
service is rate-limited. This way, we only allocate the box once when
the service is created.
There should be no actual changes in functionality.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
CI is currently busted due to [issues with caching `cargo-hack`][1].
Currently, we cache the `cargo-hack` executable to speed up builds by
avoiding the overhead of compiling it from source in every build.
Recently, `cargo-hack` has started publishing binaries on GitHub
Releases. Rather than compiling it on CI and caching it, we can just
download the binary instead. This ought to fix the build.
See also taiki-e/cargo-hack#89 and taiki-e/cargo-hack#91.
[1]: https://github.com/tower-rs/tower/runs/1425940763
The previous code used a fixed-size histogram with an upper bound of 10_000 ms
(10s). This meant that the `Hedge` middleware would display errors when used
with services that take longer than 10s to complete a response. Instead, use a
constructor that produces an auto-resizing histogram. In the future, if the
auto-resizing behavior is an issue, Tower could add a second constructor for
the Hedge middleware that allows specifying bounds, but for now this change is
transparent and avoids spurious errors.
This branch updates Tower to Tokio 0.3.
Unlike #474, this branch uses Tokio 0.3's synchronization primitives,
rather than continuing to depend on Tokio 0.2. I think that we ought to
try to use Tokio 0.3's channels whenever feasible, because the 0.2
channels have pathological memory usage patterns in some cases (see
tokio-rs/tokio#2637). @LucioFranco let me know what you think of the
approach used here and we can compare notes!
For the most part, this was a pretty mechanical change: updating
versions in Cargo.toml, tracking feature flag changes, renaming
`tokio::time::delay` to `sleep`, and so on. Tokio's channel receivers
also lost their `poll_recv` methods, but we can easily replicate that by
enabling the `"stream"` feature and using `poll_next` instead.
The one actually significant change is that `tokio::sync::mpsc::Sender`
lost its `poll_ready` method, which impacts the way `tower::buffer` is
implemeted. When the buffer's channel is full, we want to exert
backpressure in `poll_ready`, so that callers such as load balancers
could choose to call another service rather than waiting for buffer
capacity. Previously, we did this by calling `poll_ready` on the
underlying channel sender.
Unfortunately, this can't be done easily using Tokio 0.3's bounded MPSC
channel, because it no longer exposes a polling-based interface, only an
`async fn ready`, which borrows the sender. Therefore, we implement our
own bounded MPSC on top of the unbounded channel, using a semaphore to
limit how many items are in the channel.
I factored out the code for polling a semaphore acquire future from
`limit::concurrency` into its own module, and reused it in `Buffer`.
Additionally, the buffer tests needed to be updated, because they
currently don't actually poll the buffer service before calling it. This
violates the `Service` contract, and the new code actually fails as a
result.
Closes#473Closes#474
Co-authored-by: Lucio Franco <luciofranco14@gmail.com>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The `ready_cache::error::Failed` type does not actually expose its inner
error type via `Error::source`. This change fixes this and improves
debug formatting.
This change adds a new constraint, ensuring that keys impl Debug in
order for the `error::Failed` type to impl Debug/Error.
Noteworthy changes:
- All constructors now follow the same pattern: `new` uses OS entropy,
`from_rng` takes a `R: Rng` and seeds the randomness from there.
`from_rng` is fallible, since randomness generators can be fallible.
- `BalanceLayer` was renamed to `MakeBalanceLayer`, since it is not
_really_ a `BalanceLayer`. The name of `BalanceMake` was also
"normalized" to `MakeBalance`.
Another observation: the `Debug` bound on `Load::Metric` in
`p2c::Balance`, while not particularly onerous, generates really
confusing errors if you forget it include it. And crucially, the error
never points at `Debug` (should we file a compiler issue?), so I pretty
much had to guess my way to that being wrong in the doc example.
It would probably be useful to add a documentation example to
`MakeBalanceLayer` or `MakeBalance` (I suspect just one of them is fine,
since they're basically the same). Since I've never used it, and find it
hard to think of uses for it, it might be good if someone with more
experience with it wrote one.
This was a mostly mechanical change. I think in at least one place it
results in a `'static` bound being added, but the next tower release
will be breaking anyway, so that's okay.
I think it helps to also document the alias at the top to (eventually)
explain how people can interact with the error they get back to discover
the "deeper cause".
## Motivation
Commit #330 introduced a regression when porting `tower-util::Oneshot`
from `futures` 0.1 to `std::future`. The *intended* behavior is that a
oneshot future should repeatedly call `poll_ready` on the oneshotted
service until it is ready, and then call the service and drive the
returned future. However, #330 inadvertently changed the oneshot future
to poll the service _once_, call it if it is ready, and then drop it,
regardless of its readiness.
In the #330 version of oneshot, an `Option` is used to store the
request while waiting for the service to become ready, so that it can be
`take`n and moved into the service's `call`. However, the `Option`
contains both the request _and_ the service itself, and is taken the
first time the service is polled. `futures::ready!` is then used when
polling the service, so the method returns immediate if it is not ready.
This means that the service itself (and the request), which were taken
out of the `Option`, will be dropped, and if the oneshot future is
polled again, it will panic.
## Solution
This commit changes the `Oneshot` future so that only the request lives
in the `Option`, and it is only taken when the service is called, rather
than every time it is polled. This fixes the bug.
I've also added a test for this which fails against master, but passes
after this change.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This also renames the `Instrument` trait, and related types, to better
reflect what they do. Specifically, the trait is now called
`TrackCompletion`, and `NoInstrument` is called `CompleteOnResponse`.
Also brings back balance example and makes it compile.
* Change Discover to be a sealed trait
`Discover` was _really_ just a `TryStream<Item = Change>`, so this
change makes that much clearer. Specifically, users are intended to use
`Discover` only in bounds, whereas implementors should implement
`Stream` with the appropriate `Item` type. `Discover` then comes with a
blanket implementation for anything that implements `TryStream`
appropriately. This obviates the need for the `discover::stream` module.
