ready-cache: Ensure cancelation can be observed #668

olix0r · 2022-06-17T01:40:28Z

tokio::task enforces a cooperative scheduling regime that can cause
oneshot::Receiver::poll to return pending after the sender has sent an
update. ReadyCache uses a oneshot to notify pending services that they
should not become ready. When a cancelation is not observed, the ready
cache return service instances that should have been canceled, which
breaks assumptions and causes an invalid state.

Fixes #415

Co-authored-by: Eliza Weisman eliza@buoyant.io

This is based on tower-0.4.12. We probably need to make a an 0.4 branch to get a bugfix release out.

`tokio::task` enforces a cooperative scheduling regime that can cause `oneshot::Receiver::poll` to return pending after the sender has sent an update. `ReadyCache` uses a oneshot to notify pending services that they should not become ready. When a cancelation is not observed, the ready cache return service instances that should have been canceled, which breaks assumptions and causes an invalid state. Fixes #415 Co-authored-by: Eliza Weisman <eliza@buoyant.io>

tower-rs/tower#668 fixes a bug that can cause the load balancer to fail to properly observe cancelations when an endpoint is replaced or removed. This change updates the proxy to use tower from a fixed git SHA. Signed-off-by: Oliver Gould <ver@buoyant.io>

@carllerche

@carllerche points out that, even with `unconstrained`, there are no guarantees that a `oneshot::Receiver` will observe the sent value immediately, even when using `tokio::task::unconstrained`.

hawkw

this is great, i think we should go ahead and merge it, release a 0.4.13, and then get it forward-ported to master!

i commented on some very minor style nits, but there are no blockers here IMO!

tower/src/ready_cache/cache.rs

hawkw · 2022-06-17T16:21:33Z

tower/src/ready_cache/cache.rs

-        if let Poll::Ready(r) = Pin::new(&mut fut).poll(cx) {
+        // This MUST return ready as soon as the sender has been notified so
+        // that we don't return a service that has been canceled, so we disable
+        // cooperative scheduling on the receiver. Otherwise, the receiver can


tiny nit:

Suggested change

// cooperative scheduling on the receiver. Otherwise, the receiver can

// cooperative yielding on the receiver. Otherwise, the receiver can

it's all cooperative scheduling :)

hawkw · 2022-06-17T16:23:19Z

tower/src/ready_cache/cache.rs

+        // This MUST return ready as soon as the sender has been notified so
+        // that we don't return a service that has been canceled, so we disable
+        // cooperative scheduling on the receiver. Otherwise, the receiver can
+        // sporadically return pending even though the sender has fired.
+        let mut cancel =
+            tokio::task::unconstrained(self.cancel.as_mut().expect("polled after complete"));


style nit, take it or leave it: might consider something like this, so that it's very obvious that the comment is referring to the use of unconstrained:

Suggested change

// This MUST return ready as soon as the sender has been notified so

// that we don't return a service that has been canceled, so we disable

// cooperative scheduling on the receiver. Otherwise, the receiver can

// sporadically return pending even though the sender has fired.

let mut cancel =

tokio::task::unconstrained(self.cancel.as_mut().expect("polled after complete"));

let cancel = self.cancel.as_mut().expect("polled after complete");

// This MUST return ready as soon as the sender has been notified so

// that we don't return a service that has been canceled, so we disable

// cooperative scheduling on the receiver. Otherwise, the receiver can

// sporadically return pending even though the sender has fired.

let mut cancel = tokio::task::unconstrained(cancel);

Signed-off-by: Oliver Gould <ver@buoyant.io>

hawkw

i don't think this use of Notify is actually correct since we are dropping the Notified future at the end of the poll. i have a thought on what I think is the best way to do this, i can open a PR against this branch?

hawkw · 2022-06-17T16:44:05Z

tower/src/ready_cache/cache.rs

+/// A `tokio::sync::oneshot` is NOT used, as a `Receiver` is not guaranteed to
+/// observe results as soon as a `Sender` fires. Using an `AtomicBool` allows
+/// the state to be observed as soon as the cancelation is triggered.


👍 for the comment

tower/src/ready_cache/cache.rs

hawkw · 2022-06-17T16:59:11Z

@olix0r wdyt of #669 ?

This replaces the use of `tokio::sync::Notify` with `futures::task::AtomicWaker`. `Notify` requires the `Notified` future to be held in order to remain interested in the notification. Dropping the future returned by `Notify::notified` cancels interest in the notification. So, the current code using `Notify` is incorrect, as the `Notified` future is created on each poll and then immediately dropped, releasing interest in the wakeup. We could solve this by storing the `Notify::notified` future in the `Pending` future, but this would be a bit of a pain, as the `Notified` future borrows the `Notify`. I thought that it was a nicer alternative to just rewrite this code to use `AtomicWaker` instead. Also, `AtomicWaker` is a much simpler, lower-level primitive. It simply stores a single waker that can wake up a single task. `Notify` is capable of waking multiple tasks, either in order or all at once, which makes it more complex.

Signed-off-by: Oliver Gould <ver@buoyant.io>

`tokio::task` enforces a cooperative scheduling regime that can cause `oneshot::Receiver::poll` to return pending after the sender has sent an update. `ReadyCache` uses a oneshot to notify pending services that they should not become ready. When a cancelation is not observed, the ready cache return service instances that should have been canceled, which breaks assumptions and causes an invalid state. This branch replaces the use of `tokio::sync::oneshot` for canceling pending futures with a custom cancelation handle using an `AtomicBool` and `futures::task::AtomicWaker`. This ensures that canceled `Pending` services are always woken even when the task's budget is exceeded. Additionally, cancelation status is now always known to the `Pending` future, by checking the `AtomicBool` immediately on polls, even in cases where the canceled `Pending` future was woken by the inner `Service` becoming ready, rather than by the cancelation. Fixes #415 Signed-off-by: Oliver Gould <ver@buoyant.io> Co-authored-by: Eliza Weisman <eliza@buoyant.io>

# 0.4.13 (June 17, 2022) ### Added - **load_shed**: Public constructor for `Overloaded` error ([#661]) ### Fixed - **util**: Fix hang with `call_all` when the `Stream` of requests is pending ([#656]) - **ready_cache**: Ensure cancelation is observed by pending services ([#668], fixes [#415]) - **docs**: Fix a missing section header due to a typo ([#646]) - **docs**: Fix broken links to `Service` trait ([#659]) [#661]: #661 [#656]: #656 [#668]: #668 [#415]: #415 [#646]: #646 [#659]: #659

olix0r requested review from jonhoo and hawkw June 17, 2022 01:40

olix0r marked this pull request as draft June 17, 2022 01:40

This was referenced Jun 17, 2022

Outbound requests for some downstream services consistently fail after the proxy reported "panicked at 'cancel sender lost'" linkerd/linkerd2#8666

Closed

Used patched tower that avoids balancer panic linkerd/linkerd2-proxy#1753

Closed

olix0r added 3 commits June 17, 2022 15:32

Simplify use of unconstrained to restore Debug impls

163ca2b

fixup assertion

2740c23

Use an AtomicBool and Notify

daaa285

@carllerche points out that, even with `unconstrained`, there are no guarantees that a `oneshot::Receiver` will observe the sent value immediately, even when using `tokio::task::unconstrained`.

hawkw approved these changes Jun 17, 2022

View reviewed changes

Move tests out of module

16f081e

Signed-off-by: Oliver Gould <ver@buoyant.io>

hawkw requested changes Jun 17, 2022

View reviewed changes

hawkw and others added 3 commits June 17, 2022 10:01

typo

babe110

typo

1e73796

hawkw marked this pull request as ready for review June 17, 2022 18:27

Merge branch 'master' into ver/rc-notify

e79b331

hawkw changed the title ~~ready-cache: Ensure cancelation updates can be observed~~ ready-cache: Ensure cancelation can be observed Jun 17, 2022

hawkw approved these changes Jun 17, 2022

View reviewed changes

clippy

26b5f15

Signed-off-by: Oliver Gould <ver@buoyant.io>

hawkw merged commit edd922d into master Jun 17, 2022

hawkw deleted the ver/rc-notify branch June 17, 2022 19:06

hawkw mentioned this pull request Jun 17, 2022

backport non-breaking changes from master to v0.4.x #671

Merged

hawkw mentioned this pull request Jun 17, 2022

chore: prepare to release tower v0.4.13 #672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ready-cache: Ensure cancelation can be observed #668

ready-cache: Ensure cancelation can be observed #668

olix0r commented Jun 17, 2022 •

edited

hawkw left a comment

hawkw Jun 17, 2022

hawkw Jun 17, 2022

hawkw left a comment

hawkw Jun 17, 2022

hawkw commented Jun 17, 2022

	// cooperative scheduling on the receiver. Otherwise, the receiver can
	// cooperative yielding on the receiver. Otherwise, the receiver can

ready-cache: Ensure cancelation can be observed #668

ready-cache: Ensure cancelation can be observed #668

Conversation

olix0r commented Jun 17, 2022 • edited

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jun 17, 2022

Choose a reason for hiding this comment

hawkw Jun 17, 2022

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jun 17, 2022

Choose a reason for hiding this comment

hawkw commented Jun 17, 2022

olix0r commented Jun 17, 2022 •

edited