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

buffer: replace linkerd-buffer with tower::buffer from upstream #922

Merged
merged 12 commits into from
Feb 17, 2021

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Feb 17, 2021

The proxy currently has its own implementation of a tower Service
that makes an inner service Cloneable 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 <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from a team February 17, 2021 19:53
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

this looks great! a few nits...

@@ -82,6 +81,7 @@ features = [
"spawn-ready",
"timeout",
"util",
"buffer",
Copy link
Member

Choose a reason for hiding this comment

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

nit: sort

Comment on lines 46 to 47
pub type Client<B> = svc::buffer::Buffer<
svc::BoxService<http::Request<B>, http::Response<RspBody>, Error>,
Copy link
Member

Choose a reason for hiding this comment

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

tioli: consider adding to svc:

type Buffer<Req, Rsp> = buffer::Buffer<BoxService<Req, Rsp, Error>, Req>;

to simplify references like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good call

where
Req: Send + 'static,
Rsp: Send + 'static,
// Rsp: Send + 'static,
Copy link
Member

Choose a reason for hiding this comment

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

rm?

hawkw and others added 4 commits February 17, 2021 15:07
Co-authored-by: Oliver Gould <ver@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from olix0r February 17, 2021 23:38
@hawkw hawkw merged commit c30e2af into main Feb 17, 2021
@hawkw hawkw deleted the eliza/tower-buffer branch February 17, 2021 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants