This adds a new `Backoff` trait and a `ExponentialBackoff`
implementation borrwoed from `linkerd2-proxy`. This provides the initial
building blocks for a more fully batteries included retry policy.
This adds new PRNG utilities that only use libstd and not the external
`rand` crate. This change's motivation are that in tower middleware that
need PRNG don't need the complexity and vast utilities of the `rand`
crate.
This adds a `Rng` trait which abstracts the simple PRNG features tower
needs. This also provides a `HasherRng` which uses the `RandomState`
type from libstd to generate random `u64` values. In addition, there is
an internal only `sample_inplace` which is used within the balance p2c
middleware to randomly pick a ready service. This implementation is
crate private since its quite specific to the balance implementation.
The goal of this in addition to the balance middlware getting `rand`
removed is for the upcoming `Retry` changes. The `next_f64` will be used
in the jitter portion of the backoff utilities in #685.
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
This changes the `Policy` trait in the `retry` layer to accept `&mut
self` instead of `&self` and changes the output type of the returned
future to `()`. The motivation for this change is to simplify
the trait a bit. By the trait methods having mutable references it means
that for each retry request session you can mutate the local policy.
This is because the policy is cloned for each individual request that
arrives into the retry middleware. In addition, this allows the `Policy`
trait to be object safe.
- The documentation should call self.inner.poll_ready() in the
Wrapper::poll_ready() call to emphasize that self.inner may only be
ready on the original instance and not a clone of inner.
* improve flexiblity of retry policy
retry::Policy is an effective way to expressing retries, however, there two use cases that
as it stands today, cannot be expressed:
- Altering the final response (eg. to record the fact that you ran out of retries)
- Altering the request (eg. to set a header to the server indicating that this request is a retry)
(Technically the second is possible with `clone_request`, but it's a little unclear _which_ request would actually get sent).
This change implements what I think is pretty close to the minimal update to make this possible, namely, `req`
and `Res` both become mutable references. This enables policies to mutate them during execution & enables both of the
use cases above without complicating the "simple path" callers who don't need this behavior.
**This is a breaking change.** However, the fixes are only a couple of `&mut` and potentially a call to `as_ref()`.
* Update changelog
* doc updates
- Wrap docs to 80 characters
- Small doc tweaks / rewrites to clarify & remove `you`
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: Lucio Franco <luciofranco14@gmail.com>
`tokio::task` enforces a cooperative scheduling regime that can cause
`oneshot::Receiver::poll` to return pending after the sender has sent an
update. `ReadyCache` uses a oneshot to notify pending services that they
should not become ready. When a cancelation is not observed, the ready
cache return service instances that should have been canceled, which
breaks assumptions and causes an invalid state.
This branch replaces the use of `tokio::sync::oneshot` for canceling
pending futures with a custom cancelation handle using an `AtomicBool`
and `futures::task::AtomicWaker`. This ensures that canceled `Pending`
services are always woken even when the task's budget is exceeded.
Additionally, cancelation status is now always known to the `Pending`
future, by checking the `AtomicBool` immediately on polls, even in cases
where the canceled `Pending` future was woken by the inner `Service`
becoming ready, rather than by the cancelation.
Fixes#415
Signed-off-by: Oliver Gould <ver@buoyant.io>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
In many cases, new releases of a dependency can break compatibility with
Tower's minimum supported Rust version (MSRV). It shouldn't be necessary
for Tower to bump its MSRV when a dependency does, as users on older
Rust versions should be able to depend on older versions of that crate.
Instead, we should probably just run our MSRV checks with minimal
dependency versions.
This branch changes Tower's CI jobs to do that. It was also necessary to
make some changes to the `Cargo.toml` to actually fix the build with
minimal dependency versions.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This gets rid of the `Unpin` impl with the weird comment on it.
Alternatively, we could just put a `S: Unpin` bound on `Pending`, but
this changes the public API to require that the service type is `Unpin`.
In practice, it will be, but we could also just avoid the trait bound.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Per #456, there are a number of issues with the `balance::Pool` API that
limit its usability, and it isn't widely used. In the discussion on that
issue, we agreed that it should probably just be removed in 0.5 --- it
can be replaced with something more useful later.
This branch removes `balance::Pool`.
CLoses#456.
* buffer: change `Buffer` to be generic over inner service's `Future`
This commit changes the `Buffer` service to be generic over the inner
service's `Future` type, rather than over the `Service` type itself. The
`Worker` type is still generic over the inner `Service` (and, it must
be, as it stores an instance of that service). This should reduce type
complexity of buffers a bit, as they will erase nested service types.
Unfortunately, `Buffer` is still generic over the inner service's
`Future`, which may also be nested, but it is likely to be less complex
than the `Service`. It would be nice if we could erase the `Future` type
as well, but I don't believe this is possible without either boxing the
futures or changing them to always be spawned on a background task,
neither of which seems desirable here.
Closes#641
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
* rewrite `buffer` to use bounded MPSC
## Motivation
Currently, `tower::buffer` uses `tokio::sync::mpsc`'s _unbounded_
channel plus a `tokio::sync::Semaphore`, in order to re-implement a
bounded channel. This was necessary because when this code was updated
to the latest version of `tokio`, there was no way to reserve a
non-borrowed send permit from a `Sender`. Thus, it was necessary to use
the `Semaphore` for the future that is polled in `poll_ready` to acquire
send capacity, since a `Permit` from the `Sender` could not be stored in
the struct until it's consumed in `call`.
This code is Not Ideal. Reimplementing the bounded channel makes the
implementation more complicated, and means that there is a bunch of
extra stuff we have to do to e.g. propagate cancellations/service errors
to tasks waiting on `poll_ready`. The bounded MPSC would solve most of
this for us. It might also be a bit more efficient, since we would only
have a single reference-counted heap allocation (the `Sender`), rather
than two (the `Sender` _and_ the `Arc<Semaphore>`).
in `tokio-util` v0.7, the semantics of `PollSender` was changed to have
a `poll_reserve` rather than `poll_send_done`, which is the required
behavior for `Buffer`. Therefore, we can now just use the
`tokio_util::sync::PollSender` type, and the buffer implementation is
now much simpler.
## Solution
This branch changes the buffer to use only a single bounded MPSC via
`PolLSender`, rather than an unbounded MPSC and a semaphore. The bounded
MPSC internally manages its semaphore, so we can now remove a lot of
complexity in the current implementation.
I had to change some of the integration tests slightly as part of this
change. This is because the buffer implementation using semaphore
permits is _very subtly_ different from one using a bounded channel. In
the `Semaphore`-based implementation, a semaphore permit is stored in
the `Message` struct sent over the channel. This is so that the capacity
is used as long as the message is in flight. However, when the worker
task is processing a message that's been recieved from the channel, the
permit is still not dropped. Essentially, the one message actively held
by the worker task _also_ occupies one "slot" of capacity, so the actual
channel capacity is one less than the value passed to the constructor,
_once the first request has been sent to the worker_. The bounded MPSC
changed this behavior so that capacity is only occupied while a request
is actually in the channel, which broke some tests that relied on the
old (and technically wrong) behavior.
## Notes
There is one sort of significant issue with this change, which is that
it unfortunately requires adding `Send` and `'static` bounds to the
`T::Future` and `Request` types. This is because they occur within the
`Message` type that's sent over the channel, and a MPSC's
`OwnedPermit<T>` for a message of type `T` is only `Send` or `'static`
when `T` is. For `PollSender` to be `Send` + `Sync`, the
`ReusableBoxFuture` that it uses internally requires the future be `Send
+ 'static`, which it is not when `OwnedPermit` isn't.
I don't believe that it's actually _necessary_ for the `OwnedPermit<T>`
type to require `T: Send` in order to be `Send`, or `T: 'static` in
order to be valid for the `'static` lifetime. An `OwnedPermit` never
actually contains an instance of type `T`, it just represents the
_capacity_ to send that type to the channel. The channel itself will
actually contain the values of type `T`. Therefore, it's possible this
could be changed upstream in Tokio, although I haven't looked into it
yet.
This is, however, a breaking change.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
The `spawn_ready` module has two notable (public API) problems:
1. `MakeSpawnReady` is useless, as it doesn't use the target type in any
novel way. It's nothing more than a `MapResponse`.
2. The `SpawnReadyLayer` type produces a `MakeSpawnReady` (which is, as
mentioned, useless).
This change removes the `spawn_ready::make` module and modifies
`SpawnReadyLayer` to produce `SpawnReady` instances directly.
---
This is unfortunately a breaking change. However, the current state of
`SpawnReadyLayer` is so convoluted that I can't imagine anyone is
actually using it... If the breakage is untenable, I can revert the
module changes and include only the `JoinHandle` change.
In practice I've found `Either` be hard to use since it changes the
error type to `BoxError`. That means if you combine two infallible
services you get a service that, to the type system, is fallible. That
doesn't work well with [axum's error
handling](https://docs.rs/axum/latest/axum/error_handling/index.html)
model which requires all services to be infallible and thus always
return a response. So you end up having to add boilerplate just to
please the type system.
Additionally, the fact that `Either` implements `Future` also means we
cannot fully remove the dependency on `pin-project` since
`pin-project-lite` doesn't support tuple enum variants, only named
fields.
This PR reworks `Either` to address these:
- It now requires the two services to have the same error type so no
type information is lost. I did consider doing something like `where
B::Error: From<A::Error>` but I hope this simpler model will lead to
better compiler errors.
- Changes the response future to be a struct with a private enum using
`pin-project-lite`
- Removes the `Future` impl so we can remove the dependency on
`pin-project`
Goes without saying that this is a breaking change so we have to wait
until tower 0.5 to ship this.
cc @jplatte
Fixes#594Fixes#550
In #643, I accidentally included a `v` before the version number in the
regex for matching release tags for the `tower` crate, but not for
`tower-whatever` crates. All previous release tags on this repo don't
use a `v`, so adding it was a mistake. This branch removes it.
`tower` builds are now failing on CI because Tokio v1.17.0 bumped MSRV
to 1.49.0. This branch updates `tower`'s MSRV to 1.49.0 to track Tokio's
MSRV. I also added nicer documentation of the MSRV based on Tokio's, and
added the `rust-version` Cargo metadata to the `tower` crate's
`Cargo.toml`.
Note that `tower-service` and `tower-layer` can technically continue to
support much earlier Rust versions than `tower` can, since they don't
depend on external crates and are very small. We could consider testing
separate, older MSRVs on CI for those crates individually. I didn't do
that in this PR, though, because I wasn't sure if this was worth the
effort and I just wanted to get CI passing again.
This branch adds a GitHub Actions workflow to automatically publish
release notes to GitHub Releases when a tag is pushed on the `master`
branch that corresponds to a release.
The release notes are parsed from the changelog using
taiki-e/create-release-action. The workflow will only run when a tag
matching `tower-[0-9].+` or `tower-[a-z]+-[0-9].+` is pushed to the
`master` branch on the origin (`tower-rs/tower`) repo.
This PR updates `tokio-util` to v0.7.
It also updates the minimum `tokio` dependency to v1.6.0.
This is because `tokio-util` requires at least `tokio` v1.6.0 for
`mpsc::Sender::reserve_owned`, but it only specifies a minimum version
of v1.0.0. This is incorrect and should be considered an upstream bug,
but updating our tokio dep fixes this, so that should at least unbreak
`tower`'s build for now.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This fixes a bunch of minor clippy lints. None of them were particularly
major, but I was getting tired of the warnings showing up in vscode.
The one lint that had to be ignored rather than fixed is the
`clippy::bool_assert_comparison` lint, which triggers on the
`tower_test::assert_request_eq!` macro. The lint triggers when writing
code like `assert_eq!(whatever, true)` rather than simply
`assert!(whatever)`. In this case, this occurs because the macro makes
an assertion about a request value, and in _some_ tests, the request
type is `bool`. We can't change this to use `assert!`, because in most
cases, when the request is not `bool`, we actually do need `assert_eq!`,
so I ignored that warning.
Tower has test and check jobs on CI that run on a build matrix
including a number of Rust versions. By default, GitHub Actions has
fail-fast semantics for matrix jobs, so if any matrix job fails, the
rest are cancelled and the build is failed. This is intended to help
builds complete faster.
This isn't really the ideal behavior for testing across multiple Rust
versions. When a build fails on a particular toolchain version, we would
ideally like to know whether the failure is localized to that version or
exists on _all_ Rust versions. This is particularly important for builds
on nightly Rust, as the nightly toolchain is more likely to contain
compiler regressions that might not be our fault at all. Similarly, we
might want to know if a change only broke the build on our MSRV, or if
it broke the build everywhere --- such an issue would be fixed
differently.
This also currently means that the nightly test run failing will prevent
PRs from being merged, even if the failure is due to a nightly compiler
regression. We currently only *require* the stable and MSRV test runs
to pass in order to merge a PR, but because the fail-fast behavior
will cancel them if the nightly build fails, this means that nightly failing
will effectively prevent merging PRs...which, given that it's not marked
as required, seems different from what we intended.
Therefore, this PR changes the CI workflow to disable fail-fast behavior
on the cross-version test jobs.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in tower, but we have some potentialy flawed `Instant`
arithmetic that could panic in this way.
Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.
This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic. These fixes should ultimately be made in the standard library,
but this change lets us avoid this problem while we wait for those
fixes.
See also hyperium/hyper#2746
`tower` currently has required dependencies that may not be used
unless certain features are enabled.
This change updates `tower` to make these dependencies optional.
Furthermore, this change removes use of `html_root_url`, which is no
longer recommended (https://github.com/rust-lang/api-guidelines/pull/230),
and updates the documented release instructions.
The tower crates do not include any `unsafe` code, but tools like
[`cargo-geiger`][cg] can't necessarily detect that. This change adds a
stronger assertion at the top of each crate with a
`#![forbid(unsafe_code)]` directive to assert that no unsafe code is
used in the crate. This also serves as a more obvious obstacle to
introducing unsafe code in future changes.
[cg]: https://github.com/rust-secure-code/cargo-geiger
* builder,util: add convenience methods for boxing services
This adds a couple of new methods to `ServiceBuilder` and `ServiceExt`:
- `ServiceBuilder::boxed`
- `ServiceExt::boxed`
- `ServiceBuilder::clone_boxed`
- `ServiceExt::clone_boxed`
They apply `BoxService::layer` and `CloneBoxService::layer`
respectively.
* fix doc links
* add missing `cfg`s
* Update tower/CHANGELOG.md
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
* Apply suggestions from code review
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
* not sure why rustdoc cannot infer these
* line breaks
* trailing whitespace
* make docs a bit more consistent
* fix doc links
* update tokio
* don't pull in old version of tower
* Don't run `cargo deny check bans` as it hangs
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
* util: add `CloneService`
This upstreams a little utility I'm using a bunch in axum. Its often
useful to erase the type of a service while still being able to clone
it.
`BoxService` isn't `Clone` previously you had to combine it with
`Buffer` but doing that a lot (which we did in axum) had measurable
impact on performance.
* Address review feedback
* remove needless trait bounds
