update to Tokio 0.3 #476

hawkw · 2020-10-26T23:38:55Z

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 #473
Closes #474

Co-authored-by: Lucio Franco [email protected]

Signed-off-by: Eliza Weisman <[email protected]>

@LucioFranco

@LucioFranco this is the basic idea; there are a couple other things that don't compile yet. Signed-off-by: Eliza Weisman <[email protected]>

Signed-off-by: Eliza Weisman <[email protected]>

LucioFranco

LGTM just a few tracing things left over, ill close my PR :)

LucioFranco · 2020-10-27T13:45:50Z

tower/src/buffer/service.rs

-            Poll::Ready(Err(self.get_worker_error()))
-        } else {
-            Poll::Ready(Ok(()))
+        tracing::info!("poll");


I had issues with this test previously, but also by the looks of these tracing statements you did too? Probably should remove these :)

oh whoops, didn't mean to leave those in!

LucioFranco · 2020-10-27T13:49:18Z

tower/src/spawn_ready/future.rs

+        if let Poll::Ready(_) = {
+            let closed = this.tx.as_mut().expect("illegal state").closed();
+            tokio::pin!(closed);
+            closed.poll(cx)
+        } {


nit: this is a bit hard to read, can we move out some of this logic from this block?

hawkw · 2020-10-27T16:12:23Z

ill close my PR :)
@LucioFranco if there's anything you got that I missed, we could also merge one branch into the other?

Signed-off-by: Eliza Weisman <[email protected]>

LucioFranco · 2020-10-27T16:46:20Z

@hawkw only last thing is my PR did some stuff with feature flags could you ensure you just copy that? beyond the tokio 0.2 sync stuff it fixed a few things and added a full flag.

Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2020-10-27T18:01:58Z

tower/src/buffer/worker.rs

+            // If the oneshot sender is closed, then the receiver is dropped,
+            // and nobody cares about the response. If this is the case, we
+            // should continue to the next request.
+            if !msg.tx.is_closed() {


n.b. that tokio 0.3.2 is adding back oneshot::Sender::poll_closed, but I don't think we actually need to poll here, since we immediately consume the sender if it isn't closed. we don't need to register interest in it closing, since we're not going to yield until we're done with that sender...so is_closed is probably slightly more efficient, too.

Signed-off-by: Eliza Weisman <[email protected]>

LucioFranco

Awesome stuff @hawkw thanks for getting this done!

LucioFranco · 2020-10-27T18:11:41Z

tower/src/spawn_ready/future.rs

+        // rather than just calling `is_closed` on it, since we want to be
+        // notified if the receiver is dropped.
+        let closed = {
+            // TODO(eliza): once `tokio` 0.3.2 is released, we can change this back


open an issue for this?

…922) The proxy currently has its own implementation of a `tower` `Service` that makes an inner service `Clone`able by driving it in a spawned task and buffering requests on a channel. This also exists upstream, as `tower::buffer`. We implemented our own version for a couple of reasons: to avoid an upstream issue where memory was leaked when a buffered request was cancelled, and to implement an idle timeout when the buffered service has been unready for too long. However, it's no longer necessary to reimplement our own buffer service for these reasons: the upstream bug was fixed in `tower` 0.4 (see tower-rs/tower#476, tower-rs/tower#480, and tower-rs/tower#556); and we no longer actually use the buffer idle timeout (instead, we idle out unresponsive services with the separate `Failfast` middleware, note that `push_spawn_buffer_with_idle_timeout` is never actually used). Therefore, we can remove our _sui generis_ implementation in favour of `tower::buffer` from upstream. This eliminates dead code for the idle timeout, which we never actually use, and reduces duplication (since `tonic` uses `tower::buffer` internally, its code is already compiled into the proxy). It also reduces the amount of code I'm personally responsible for maintaining in two separate places ;) Since the `linkerd-buffer` crate erases the type of the buffered service, while `tower::buffer` does not, I've changed the `push_spawn_buffer`/`spawn_buffer` helpers to also include a `BoxService` layer. This required adding a `BoxServiceLayer` type, since `BoxService::layer` returns a `LayerFn` with an unnameable type. Also, this change ran into issues due to a compiler bug where generators (async blocks) sometimes forget concrete lifetimes, rust-lang/rust#64552. In order to resolve this, I had to remove the outermost `async` blocks from the OpenCensus and identity daemon tasks. These async blocks were used only for emitting a tracing event when the task is started, so it wasn't a big deal to remove them; I moved the trace events into the actual daemon task functions, and used a `tracing` span to propagate the remote addresses which aren't known inside the daemon task functions. Signed-off-by: Eliza Weisman <[email protected]>

hawkw added 9 commits October 26, 2020 13:49

update all the easy stuff to tokio 0.3

6f22d61

Signed-off-by: Eliza Weisman <[email protected]>

factor out concurrency limit semaphore wrapper

e970668

Signed-off-by: Eliza Weisman <[email protected]>

use semaphore in buffer

6c7b5bc

@LucioFranco this is the basic idea; there are a couple other things that don't compile yet. Signed-off-by: Eliza Weisman <[email protected]>

update buffer

f4e6e2e

Signed-off-by: Eliza Weisman <[email protected]>

update spawn_ready

2c73235

Signed-off-by: Eliza Weisman <[email protected]>

lol tests never call poll_ready

4bf1153

Signed-off-by: Eliza Weisman <[email protected]>

test that bound actually works

50c44f2

Signed-off-by: Eliza Weisman <[email protected]>

clean up warnings

5200b9a

Signed-off-by: Eliza Weisman <[email protected]>

docs

28fe32e

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested review from carllerche, jonhoo and LucioFranco October 26, 2020 23:38

hawkw self-assigned this Oct 26, 2020

LucioFranco requested changes Oct 27, 2020

View reviewed changes

LucioFranco mentioned this pull request Oct 27, 2020

Upgrade to tokio 0.3 #474

Closed

rm debug traces, add comments

2756737

Signed-off-by: Eliza Weisman <[email protected]>

hawkw and others added 4 commits October 27, 2020 09:46

rename Semaphore::poll_ready -> poll_acquire

2ad8bce

Signed-off-by: Eliza Weisman <[email protected]>

fix semaphore looping twice

3921c5c

Signed-off-by: Eliza Weisman <[email protected]>

fix imports and feature flags

60dafdf

fix deny

75fc940

hawkw requested a review from LucioFranco October 27, 2020 17:01

hawkw added 3 commits October 27, 2020 10:03

undo cherry-pick mistake

ff456fb

Signed-off-by: Eliza Weisman <[email protected]>

hopefully fix deny failures

d371ae1

Signed-off-by: Eliza Weisman <[email protected]>

just skip the specific duplicate dep

d48c970

Signed-off-by: Eliza Weisman <[email protected]>

hawkw mentioned this pull request Oct 27, 2020

chore: downgrade multiple-versions to warn in cargo-deny #477

Closed

hawkw added 3 commits October 27, 2020 10:49

update to a consistent pin-project version

d54a8e1

Signed-off-by: Eliza Weisman <[email protected]>

bump tower-test, fix deny config

ff3dc34

Signed-off-by: Eliza Weisman <[email protected]>

fix buffer test that isn't quite deterministic

0cddf0f

Signed-off-by: Eliza Weisman <[email protected]>

hawkw force-pushed the eliza/tokio-0.3 branch from 88feb8e to 0cddf0f Compare October 27, 2020 17:57

hawkw commented Oct 27, 2020

View reviewed changes

spawn_ready cleanup

fd60d1a

Signed-off-by: Eliza Weisman <[email protected]>

LucioFranco approved these changes Oct 27, 2020

View reviewed changes

hawkw merged commit ddc64e8 into master Oct 27, 2020

hawkw deleted the eliza/tokio-0.3 branch October 27, 2020 18:21

This was referenced Oct 27, 2020

buffer: wake tasks waiting for channel capacity when terminating #480

Merged

sync: consider exposing Semaphore::close in public API tokio-rs/tokio#3061

Closed

fanatid mentioned this pull request Nov 30, 2020

Upgrade dependencies to tokio 1.0 vectordotdev/vector#5175

Closed

28 tasks

hawkw mentioned this pull request Dec 4, 2020

update the proxy to use Tokio 0.3 linkerd/linkerd2-proxy#732

Merged

This was referenced Feb 11, 2021

Make sure Zebra uses poll_ready and Buffer reservations correctly ZcashFoundation/zebra#1593

Closed

Prevent PeerSet service to be buffered ZcashFoundation/zebra#1718

Closed

Prevent Clients from being Buffered in zebra-network ZcashFoundation/zebra#1697

Closed

hawkw mentioned this pull request Feb 17, 2021

buffer: replace linkerd-buffer with tower::buffer from upstream linkerd/linkerd2-proxy#922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update to Tokio 0.3 #476

update to Tokio 0.3 #476

hawkw commented Oct 26, 2020 •

edited

Loading

LucioFranco left a comment

LucioFranco Oct 27, 2020

hawkw Oct 27, 2020

LucioFranco Oct 27, 2020

hawkw commented Oct 27, 2020

LucioFranco commented Oct 27, 2020

hawkw Oct 27, 2020

LucioFranco left a comment

LucioFranco Oct 27, 2020

update to Tokio 0.3 #476

update to Tokio 0.3 #476

Conversation

hawkw commented Oct 26, 2020 • edited Loading

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Oct 27, 2020

Choose a reason for hiding this comment

hawkw Oct 27, 2020

Choose a reason for hiding this comment

LucioFranco Oct 27, 2020

Choose a reason for hiding this comment

hawkw commented Oct 27, 2020

LucioFranco commented Oct 27, 2020

hawkw Oct 27, 2020

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Oct 27, 2020

Choose a reason for hiding this comment

hawkw commented Oct 26, 2020 •

edited

Loading