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.
* util: Add ReadyAnd to do what Ready should do
`ServiceExt::ready` says that it produces "A future yielding the service
when it is ready to accept a request." This is not true; it does _not_
yield the service when it is ready, it yields unit. This makes it
impossible to chain service ready with service call, which is sad.
This PR adds `ready_and`, which does what `ready` promised. It also
deprecates `ready` with the intention that we remove `ready` in a future
version, and make the strictly more general `ready_and` take its place.
We can't do it now since it's not a backwards-compatible change even
though it _probably_ wouldn't break any code.
The PR also updates the docs so that they reflect the observed behavior.
Updated the README, fixing the links to documentation. This now links
to each packages documentation on docs.rs. Not all packages have been
released to crates.io, so their documentation pages are empty.
It's been observed that occasionally tower-ready-cache would panic
trying to find an already canceled service in `cancel_pending_txs`
(#415). The source of the race is not entirely clear, but extensive
debugging demonstrated that occasionally a call to `evict` would send on
the `CancelTx` for a service, yet that service would be yielded back
from `pending` in `poll_pending` in a non-`Canceled` state. This
is equivalent to saying that this code may panic:
```rust
async {
let (tx, rx) = oneshot::channel();
tx.send(42).unwrap();
yield_once().await;
rx.try_recv().unwrap(); // <- may occasionally panic
}
```
I have not been able to demonstrate a self-contained example failing in
this way, but it's the only explanation I have found for the observed
bug. Pinning the entire runtime to one core still produced the bug,
which indicates that it is not a memory ordering issue. Replacing
oneshot with `mpsc::channel(1)` still produced the bug, which indicates
that the bug is not with the implementation of `oneshot`. Logs also
indicate that the `ChannelTx` we send on in `evict()` truly is the same
one associated with the `ChannelRx` polled in `Pending::poll`, so we're
not getting our wires crossed somewhere. It truly is bizarre.
This patch resolves the issue by considering a failure to find a
ready/errored service's `CancelTx` as another signal that a service has
been removed. Specifically, if `poll_pending` finds a service that
returns `Ok` or `Err`, but does _not_ find its `CancelTx`, then it
assumes that it must be because the service _was_ canceled, but did not
observe that cancellation signal.
As an explanation, this isn't entirely satisfactory, since we do not
fully understand the underlying problem. It _may_ be that a canceled
service could remain in the pending state for a very long time if it
does not become ready _and_ does not see the cancellation signal (so it
returns `Poll::Pending` and is not removed). That, in turn, might cause
an issue if the driver of the `ReadyCache` then chooses to re-use a key
they believe they have evicted. However, any such case _must_ first hit
the panic that exists in the code today, so this is still an improvement
over the status quo.
Fixes#415.
When ready-cache was upgraded from futures 0.1 to `std::future` in
e2f1a49cf3, this `expect` was removed, and
the code instead silently ignores the error. That's probably not what we
want, so this patch restores that assertion.
The assertion there isn't even true anyway, since the p2c may not yet
have "seen" the removal of a service, because it stopped when it found a
ready service.
This is of particular importance since the `log` feature of `tracing`
(currently) fails to compile if the `tracing` dependency is renamed.
Without a way to disable it in `tower`, any package that both depends on
`tower` **and** renames `tracing` in its dependencies is doomed.
Steven Fackler
Jon Gjengset
Lucio Franco
Akshay Narayan
Jake Ham
László Nagy
Sean McArthur
Juan Alvarez