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 linkerd2-cache and linkerd2-lock to std::future #490

Merged
merged 15 commits into from
May 4, 2020

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 24, 2020

This branch updates the linkerd2-cache crate (and linkerd2-lock,
which it relies on), to use std::future.

Unlike previous PRs, the linkerd2-lock update is a more substantial
rewrite, because upstream API changes made the previous implementation
no longer possible. In particular, tokio::sync::Mutex does not provide
a poll_acquire method the way that tokio_sync::Lock did in Tokio
0.1. The removal of poll_acquire is largely due to the rewrite of
Tokio's synchronization primitives to use an intrusive linked list-based
semaphore (tokio-rs/tokio#2325), which requires waiters to be pinned to
ensure safety of the intrusive list. Allowing permits to be acquired
only from a future ensures correct pinning of waiters.

Therefore, I've implemented a new Lock for Linkerd which uses a Tokio
Semaphore with 1 permit to ensure exclusive access. The Lock owns a
boxed instance of the future returned by Semaphore::acquire, and
drives it in poll_acquire. This allows adapting the poll-based Tower
interface with the futures-only, pinned interface.

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 April 24, 2020 21:42
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 24, 2020
This branch updates the `concurrency-limit` middleware to std::future.
The new implementation uses the new owned semaphore permit API added in
Tokio 0.2.19 (tokio-rs/tokio#2421). Similarly to #490, the old
implementation could not be directly translated due to `tokio::sync`'s
`Semaphore` losing its `poll`-based API in 0.2.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 24, 2020
This branch updates the `concurrency-limit` middleware to std::future.
The new implementation uses the new owned semaphore permit API added in
Tokio 0.2.19 (tokio-rs/tokio#2421). Similarly to #490, the old
implementation could not be directly translated due to `tokio::sync`'s
`Semaphore` losing its `poll`-based API in 0.2.

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>
@olix0r
Copy link
Member

olix0r commented Apr 27, 2020

@hawkw looks like Cargo.lock is out of date?

@@ -44,7 +44,7 @@ tower = "0.1"
tower-grpc = { version = "0.1", default-features = false, features = ["protobuf"] }
tracing = "0.1.9"
tracing-futures = "0.1"
webpki = "0.21"
webpki = "=0.21.0"
Copy link
Member

Choose a reason for hiding this comment

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

BTW, i published an updated version of our fork a while ago https://github.com/linkerd/webpki/tree/cert-dns-names-0.21

@olix0r
Copy link
Member

olix0r commented Apr 27, 2020

@hawkw Would we still need the specialized Lock implementation if we altered the Cache implementation to avoid backpressure (i.e. by cloning the mutex into the response future)?

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.

I'm OK with this merging when it's green. I think, ideally, we ought to get rid of our lock implementation.... but that's not a blocker.

@hawkw
Copy link
Contributor Author

hawkw commented Apr 27, 2020

@hawkw Would we still need the specialized Lock implementation if we altered the Cache implementation to avoid backpressure (i.e. by cloning the mutex into the response future)?

If we wanted to use Tokio's Mutex, it might also need to grow an owned-guard API like Semaphore has (which I can open a PR for). But, yeah, we could definitely do it that way (and it might be much nicer); I had kind of assumed that having Cache acquire the lock in poll_ready was a behavior we wanted to keep.

@olix0r
Copy link
Member

olix0r commented Apr 27, 2020

@hawkw I'd have to check to confirm, but I believe we wrap all of these with oneshot, effectively

hawkw added a commit that referenced this pull request Apr 27, 2020
This branch updates the `concurrency-limit` middleware to std::future.
The new implementation uses the new owned semaphore permit API added in
Tokio 0.2.19 (tokio-rs/tokio#2421). Similarly to #490, the old
implementation could not be directly translated due to `tokio::sync`'s
`Semaphore` losing its `poll`-based API in 0.2.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Looks like Cargo.lock still needs to be updated but changes look good!

@hawkw
Copy link
Contributor Author

hawkw commented Apr 29, 2020

@hawkw Would we still need the specialized Lock implementation if we altered the Cache implementation to avoid backpressure (i.e. by cloning the mutex into the response future)?

If we wanted to use Tokio's Mutex, it might also need to grow an owned-guard API like Semaphore has (which I can open a PR for). But, yeah, we could definitely do it that way (and it might be much nicer); I had kind of assumed that having Cache acquire the lock in poll_ready was a behavior we wanted to keep.

tokio-rs/tokio#2455 merged upstream, so we could definitely do this. Also, we can now remove all the unsafe in this PR and use Tokio's mutex directly, even if we did decide to continue driving to readiness in poll_ready.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from olix0r April 30, 2020 17:40
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.

lgtm, modulo the unrelated comment change

linkerd/service-profiles/src/http/service.rs Outdated Show resolved Hide resolved
hawkw added 5 commits May 4, 2020 09:44
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 merged commit a35ae86 into master-tokio-0.2 May 4, 2020
@olix0r olix0r deleted the eliza/0.2-cache branch May 25, 2021 15:47
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.

3 participants