Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update buffers to use Tokio 0.3 MPSC channels #759

Merged
merged 5 commits into from Dec 4, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Dec 4, 2020

This branch updates linkerd2-buffer, and linkerd2-proxy-discover's
buffer module to use Tokio 0.3's MPSC channel rather than Tokio 0.2's.
The rest of the proxy still uses Tokio 0.2, including the 0.2 runtime.

Most of the Tokio synchronization primitives lost their poll-based
interfaces in 0.3 as part of the move to intrusive lists of wakers for
synchronization primitives (see tokio-rs/tokio#2325,
tokio-rs/tokio#2509, and tokio-rs/tokio#2861). This change takes
advantage of the inherently pinned nature of async fn and async
blocks to avoid needing a separate heap allocation to store the waiter
state for a task waiting on a synchronization primitive. However, it
means that a synchronization primitive can only be waited on when the
future that waits on it is pinned --- otherwise, there is a potential
dangling pointer. The poll-based APIs allowed waiting on
synchronization primitives from unpinned contexts, so they were removed.

To wait on the synchronization primitives from contexts that may not be
pinned, such as poll_ready, it's necessary to add a Pin<Box<...>>
around the future that's waiting on the synchronization primitive. This
ensures that the future will not move while it's part of the wait list.
It's important to note that this isn't an additional allocation per
waiter versus Tokio 0.2; instead, it's the same allocation that would
have always happened internally to the synchronization primitive in
the 0.2 API. Now, it's moved outside of the tokio::sync type so that
it can be avoided when used with async/await syntax, and added by
the user when polling the sync primitives.

Because we need to poll channel senders in tower::Service
implementations' poll_ready functions, it was necessary to introduce
our own bounded MPSC channel type that exposes a polling-based API. 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. This
branch adds a new linkerd2-channel crate that implements a pollable
bounded channel, wrapping tokio::sync's unbounded MPSC and using a
tokio::sync::Semaphore to implement bounding. It's worth noting that
this is, essentially, how tokio::sync::mpsc's bounded channel is
implemented --- it also uses the semaphore. However, our implementation
exposes a poll_ready method by boxing the future that waits to acquire
a semaphore permit, which the Tokio channel does not expose.

Finally, I've added some tests for the linkerd2-channel crate, based
on Tokio's tests for the MPSC channel, modified where the APIs differ.
This should help ensure we get similar behavior to what we expect from
Tokio's MPSCs.

This was factored out of PR #732.

Signed-off-by: Eliza Weisman eliza@buoyant.io

This branch updates `linkerd2-buffer`, and `linkerd2-proxy-discover`'s
`buffer` module to use Tokio 0.3's MPSC channel rather than Tokio 0.2's.
The rest of the proxy still uses Tokio 0.2, including the 0.2 runtime.

Most of the Tokio synchronization primitives lost their `poll`-based
interfaces in 0.3 as part of the move to intrusive lists of wakers for
synchronization primitives (see tokio-rs/tokio#2325,
tokio-rs/tokio#2509, and tokio-rs/tokio#2861). This change takes
advantage of the inherently pinned nature of `async fn` and `async`
blocks to avoid needing a separate heap allocation to store the waiter
state for a task waiting on a synchronization primitive. However, it
means that a synchronization primitive can _only_ be waited on when the
future that waits on it is pinned --- otherwise, there is a potential
dangling pointer. The `poll`-based APIs allowed waiting on
synchronization primitives from unpinned contexts, so they were removed.

To wait on the synchronization primitives from contexts that may not be
pinned, such as `poll_ready`, it's necessary to add a `Pin<Box<...>>`
around the future that's waiting on the synchronization primitive. This
ensures that the future will not move while it's part of the wait list.
It's important to note that this isn't an _additional_ allocation per
waiter versus Tokio 0.2; instead, it's the same allocation that would
have _always_ happened internally to the synchronization primitive in
the 0.2 API. Now, it's moved outside of the `tokio::sync` type so that
it can be avoided when used with `async`/`await` syntax, and added by
the user when polling the sync primitives.

Because we need to poll channel senders in `tower::Service`
implementations' `poll_ready` functions, it was necessary to introduce
our own bounded MPSC channel type that exposes a polling-based API. 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. This
branch adds a new `linkerd2-channel` crate that implements a pollable
bounded channel, wrapping `tokio::sync`'s unbounded MPSC and using a
`tokio::sync::Semaphore` to implement bounding. It's worth noting that
this is, essentially, how `tokio::sync::mpsc`'s bounded channel is
implemented --- it also uses the semaphore. However, our implementation
exposes a `poll_ready` method by boxing the future that waits to acquire
a semaphore permit, which the Tokio channel does not expose.

This was factored out of PR #732.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested review from olix0r, zaharidichev and a team December 4, 2020 19:03
@hawkw hawkw self-assigned this Dec 4, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
linkerd/buffer/src/dispatch.rs Outdated Show resolved Hide resolved
linkerd/buffer/src/dispatch.rs Outdated Show resolved Hide resolved
@@ -54,7 +54,7 @@ pub(crate) async fn run<S, Req, I>(
e = idle().fuse() => {
let error = ServiceError(Arc::new(e.into()));
trace!(%error, "Idling out inner service");
return;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
break;
return;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

way ahead of you, buddy e2046e3 :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hawkw i think that got the other break, but there were two.. so i think this is still relevant?

@hawkw hawkw merged commit 1d64a11 into main Dec 4, 2020
@hawkw hawkw deleted the eliza/0.3-channel branch December 4, 2020 19:50
hawkw added a commit that referenced this pull request Dec 4, 2020
This branch updates the proxy to use Tokio 0.3 and the Tokio 0.3
versions of various ecosystem crates. This includes `tower` 0.4 and
`bytes` 0.6, as well as the Tokio 0.3 versions of `tokio-util`, `hyper`,
`tonic`, etc. Due to API changes in Tokio and in other dependencies, it
was necessary to make some code changes as well as updating
dependencies, but there should be no functional change.

In particular:
* Tokio's support for vectored IO changed significantly in 0.3, so this
  branch updates our use of `AsyncWrite` to participate in the new
  vectored write APIs
* Hyper's HTTP/1.1 upgrade API changed in 0.14, so this branch changes the
  proxy's code for handling CONNECT to use the new API
* Tokio removed support for some socket options, which now need to be
  set using `socket2`
* Tokio removed the `poll_ready` method was removed from the bounded
  MPSC channel, so the proxy's buffers (`linkerd2-buffer` and the
  `buffer` module in `linkerd2-proxy-discover`) had to be switched to
  our own implementation (this merged separately, in PR #759).

Several ecosystem crates have yet to be released, so we depend on them
via Git dependencies for now. The patches in Cargo.toml can be
removed as other dependencies publish their Tokio 0.3 versions.
olix0r pushed a commit that referenced this pull request Dec 4, 2020
This change updates the proxy to use Tokio 0.3 and the Tokio 0.3
versions of various ecosystem crates. This includes `tower` 0.4 and
`bytes` 0.6, as well as the Tokio 0.3 versions of `tokio-util`, `hyper`,
`tonic`, etc. Due to API changes in Tokio and in other dependencies, it
was necessary to make some code changes as well as updating
dependencies, but there should be no functional change.

In particular:
* Tokio's support for vectored IO changed significantly in 0.3, so this
  branch updates our use of `AsyncWrite` to participate in the new
  vectored write APIs
* Hyper's HTTP/1.1 upgrade API changed in 0.14, so this branch changes the
  proxy's code for handling CONNECT to use the new API
* Tokio removed support for some socket options, which now need to be
  set using `socket2`
* Tokio removed the `poll_ready` method was removed from the bounded
  MPSC channel, so the proxy's buffers (`linkerd2-buffer` and the
  `buffer` module in `linkerd2-proxy-discover`) had to be switched to
  our own implementation (this merged separately, in PR #759).

Several ecosystem crates have yet to be released, so we depend on them
via Git dependencies for now. The patches in Cargo.toml can be
removed as other dependencies publish their Tokio 0.3 versions.
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Dec 15, 2020
This release features a change to the proxy's cache eviction strategy to
ensure that clients (and their load balancers) are reused by new
outbound connections. This can dramatically reduce memory consumption,
especially for busy HTTP/1.1 clients.

Also, the proxy's HTTP detection scheme has been made more robust.
Previously, the proxy would perform a only single read to determine
whether a TCP stream was HTTP, which could lead to false positives. Now,
the proxy reads until at least the first newline, which is what the HTTP
parser actually needs to make a proper determination. With this, the
default dispatch timeouts have been increased to 5s to accomodate
connection pools that may not issue an immediate request.

Furthermore, this release includes an upgrade to Tokio v0.3 and its
associated ecosystem.

---

* update buffers to use Tokio 0.3 MPSC channels (linkerd/linkerd2-proxy#759)
* Update the proxy to use Tokio 0.3  (linkerd/linkerd2-proxy#732)
* Rename DetectHttp to NewServeHttp (linkerd/linkerd2-proxy#760)
* http: more consistent names for body types (linkerd/linkerd2-proxy#761)
* io: simplify the `Io` trait (linkerd/linkerd2-proxy#762)
* trace: nicer traces in tests, clean up trace configuration (linkerd/linkerd2-proxy#766)
* Ensure that services are held as long they are being used (linkerd/linkerd2-proxy#767)
* outbound: add stack tests for http (linkerd/linkerd2-proxy#765)
* cache: Ensure that actively held services are not evicted (linkerd/linkerd2-proxy#768)
* cache: Only spawn a single task per cache entry (linkerd/linkerd2-proxy#770)
* test: make integration tests shut up (linkerd/linkerd2-proxy#771)
* metrics: Add support for microsecond counters (linkerd/linkerd2-proxy#772)
* Add a protocol label to stack metrics (linkerd/linkerd2-proxy#773)
* detect: Make protocol detection more robust (linkerd/linkerd2-proxy#744)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Dec 15, 2020
This release features a change to the proxy's cache eviction strategy to
ensure that clients (and their load balancers) are reused by new
outbound connections. This can dramatically reduce memory consumption,
especially for busy HTTP/1.1 clients.

Also, the proxy's HTTP detection scheme has been made more robust.
Previously, the proxy would perform a only single read to determine
whether a TCP stream was HTTP, which could lead to false positives. Now,
the proxy reads until at least the first newline, which is what the HTTP
parser actually needs to make a proper determination. With this, the
default dispatch timeouts have been increased to 5s to accomodate
connection pools that may not issue an immediate request.

Furthermore, this release includes an upgrade to Tokio v0.3 and its
associated ecosystem.

---

* update buffers to use Tokio 0.3 MPSC channels (linkerd/linkerd2-proxy#759)
* Update the proxy to use Tokio 0.3  (linkerd/linkerd2-proxy#732)
* Rename DetectHttp to NewServeHttp (linkerd/linkerd2-proxy#760)
* http: more consistent names for body types (linkerd/linkerd2-proxy#761)
* io: simplify the `Io` trait (linkerd/linkerd2-proxy#762)
* trace: nicer traces in tests, clean up trace configuration (linkerd/linkerd2-proxy#766)
* Ensure that services are held as long they are being used (linkerd/linkerd2-proxy#767)
* outbound: add stack tests for http (linkerd/linkerd2-proxy#765)
* cache: Ensure that actively held services are not evicted (linkerd/linkerd2-proxy#768)
* cache: Only spawn a single task per cache entry (linkerd/linkerd2-proxy#770)
* test: make integration tests shut up (linkerd/linkerd2-proxy#771)
* metrics: Add support for microsecond counters (linkerd/linkerd2-proxy#772)
* Add a protocol label to stack metrics (linkerd/linkerd2-proxy#773)
* detect: Make protocol detection more robust (linkerd/linkerd2-proxy#744)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants