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
The balancer types derive `Debug` and `Clone` implementations, but
this unnecessarily requires that its type parameters implement these
traits.
This change provides manual implementations for `Clone` and `Debug`
to avoid this unintentional restriction.
Closes#606
* tower: prepare to release 0.4.10
- Fix accidental breaking change when using the
`rustdoc::broken_intra_doc_links` lint ([#605])
- Clarity that tower's minimum supported rust version is 1.46 ([#605])
[#605]: https://github.com/tower-rs/tower/pull/605
* Update tower/CHANGELOG.md
Co-authored-by: Oliver Gould <ver@buoyant.io>
Co-authored-by: Oliver Gould <ver@buoyant.io>
* Actually check MSCV on CI
* check broken_intra_doc_links on CI
* clean up CI
* fix other crates
* bump MSRV to 1.42 because of tracing-core
* attempt to fix http not working on 1.42
* use `--workspace` instead of `--all`
`--all` is deprecated
* make tower-service build on 1.42
* force http version 0.2.4
* actually msrv is 1.46 because of tokio
* fix running `cargo fmt`
* clean up
* also run tests on 1.46
* ignore rustsec in time
* REMOVE ME updates peak_wema test to pass
* adds pin_project_lite dependency
* uses pin_project_lite for load::Constant
* uses pin_project_lite for load::PencingRequestsDiscover
* uses pin_project_lite for load::PeakEwma
* uses pin_project_lite for load::Completion
* uses pin_project_lite for tests::support::IntoStream
Turns IntoStream into a regular struct because pin_project_lite does not and will support tuple structs.
416be96f77/src/lib.rs (L401-L408)
* refactors opaque_future into a regular struct
This enables migration to pin_project_lite, which does not and will not support tuple structs
416be96f77/src/lib.rs (L401-L408)
* migrates opaque_future to use pin_project_lite
* removes tuple variant from load_shed::ResponseState enum
* migrates load_shed::future to pin_project_lite
* removes tuple variant from filter::future::State
* migrates filter::future to pin_project_lite
Note: the doc comment on AsyncResponseFuture::service was also reduced to a regular comment.
This is a known limitation of pin_project_lite that the they have labeled as "help wanted".
https://github.com/taiki-e/pin-project-lite/issues/3#issuecomment-745194112
* migrates retry::Retry to pin_project_lite
* refactors retry::future::State to enable pin_project_lite
pin_project_lite has the current limitation of nto supporting doc comments
https://github.com/taiki-e/pin-project-lite/issues/3#issuecomment-745194112
pin_project_lite does not and will not support tuple variants
416be96f77/src/lib.rs (L401-L408)
* migrates retry::future to pin_project_lite
* migrates spawn_ready::make to pin_project_lite
* refactors buffer::future::ResponseState to allow pin_project_lite
* migrates buffer::future to pin_project_lite
* refactors util::AndThenFuture to allow pin_project_lite
* migrates util::AndThenFuture to pin_project_lite
* migrates hedge::Future to pin_project_lite
* migrates hedge::select::ResponseFuture to pin_project_lite
* refactors hedge::delay enum for pin_project_lite
* refactors reconnect::future enum for pin_project_lite
* refactors oneshot::State enum for pin_project_lite
* migrates util::oneshot to pin_project_lite
* migrates reconnect::future to pin_project_lite
* migrates hedge::delay to pin_project_lite
* migrates hedge::latency to pin_project_lite
* migrates discover::list to pin_project_lite
* migrates timeout::future to pin_project_lite
* migrates balance::pool to pin_project_lite
* migrates balance::p2c::make to pin_project_lite
* migrates balance::p2c::service to pin_project_lite
* migrates call_all::ordered to pin_project_lite
* migrates call_all::common to pin_project_lite
* migrates call_all::unordered to pin_project_lite
* migrates util::optional::future to pin_project_lite
* migrates limit::concurrency::future to pin_project_lite
* migrates tower-balance example to pin_project_lite
* applies cargo fmt
* migrates tower-test to pin_project_lite
* fixes cargo hack check
peak_wma and pending_requests will now properly compile without the "discover" feature enabled.
* fixes lint rename warning on nightly
broken_intra_doc_links has been renamed to rustdoc::broken_intra_doc_links
* migrates buffer::Worker to pin_project_lite
pin_project_lite does support PinnedDrop
https://github.com/taiki-e/pin-project-lite/pull/25/files
However, it does not support generic trait bounds on the PinnedDrop impl.
To workaround this, I removed the T::Error bound from the Worker struct definition,
and moved `close_semaphore` to a a new impl without that trait bound.
* fixes abort_on_drop test
This test was also failing on master.
* applies cargo fmt
This adds a guide that explains how to implement a middleware from scratch without taking any shortcuts. It walks through implementing `Timeout` as it exists in Tower today.
The hope is that once users have read [the previous guide](https://tokio.rs/blog/2021-05-14-inventing-the-service-trait) followed by this one they should be fully equipped to implement their own middleware.
* limit: global concurrency limit layer from a owned semaphore
* new_owned -> new_shared + docs improvements
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
* keep exposing Semaphore, but rename the API a bit and make it simpler to use
* missed a spot
* minor docs fixes
* update changelog
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
This adds the first Tower guide called "Inventing the `Service` trait". It attempts to motivate all the parts to `Service` by walking the user through how they could have invented `Service` themselves, from scratch. It goes into quite a bit of detail but hopefully it paints a somewhat complete picture in the end.
The next guide I want to write is about how to implement a proper `Timeout` middleware using `Layer`, pin-project, and all the bells and whistles.
Ref: https://github.com/tower-rs/tower/issues/33
