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

util: fix oneshot dropping pending services immediately #447

Merged
merged 2 commits into from
Apr 23, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 23, 2020

Motivation

Commit #330 introduced a regression when porting tower-util::Oneshot
from futures 0.1 to std::future. The intended behavior is that a
oneshot future should repeatedly call poll_ready on the oneshotted
service until it is ready, and then call the service and drive the
returned future. However, #330 inadvertently changed the oneshot future
to poll the service once, call it if it is ready, and then drop it,
regardless of its readiness.

In bthe #330 version of oneshot, an Option is used to store the
request while waiting for the service to become ready, so that it can be
taken and moved into the service's call. However, the Option
contains both the request and the service itself, and is taken the
first time the service is polled. futures::ready! is then used when
polling the service, so the method returns immediate if it is not ready.
This means that the service itself (and the request), which were taken
out of the Option, will be dropped, and if the oneshot future is
polled again, it will panic.

Solution

This commit changes the Oneshot future so that only the request lives
in the Option, and it is only taken when the service is called, rather
than every time it is polled. This fixes the bug.

I've also added a test for this which fails against master, but passes
after this change.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw self-assigned this Apr 23, 2020
@hawkw hawkw merged commit 8752a38 into master Apr 23, 2020
@hawkw hawkw deleted the eliza/im-not-throwin-away-my-oneshot branch April 23, 2020 23:07
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Apr 24, 2020
Tower is currently being reorganized (all the sub-crates are being
merged into feature-flagged modules of the main `tower` crate). This
hasn't been released yet, but it has landed on git master. In addition,
we need to pick up an upstream fix for a regression in the future
returned by `ServiceExt::oneshot` (tower-rs/tower#447), which is
currently unreleased; without this fix, a lot of stuff is broken and
it's difficult to test changes in other linkerd crates.

This branch updates everything that currently depends on `tower` 0.3 to
use the same dependency, and enables the features that each crate
currently uses in that crate's dependency. I've added a single patch to
use the git version in the main workspace `Cargo.toml`; when the changes
we need are published, the patch can be removed.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Apr 24, 2020
Tower is currently being reorganized (all the sub-crates are being
merged into feature-flagged modules of the main `tower` crate). This
hasn't been released yet, but it has landed on git master. In addition,
we need to pick up an upstream fix for a regression in the future
returned by `ServiceExt::oneshot` (tower-rs/tower#447), which is
currently unreleased; without this fix, a lot of stuff is broken and
it's difficult to test changes in other linkerd crates.

This branch updates everything that currently depends on `tower` 0.3 to
use the same dependency, and enables the features that each crate
currently uses in that crate's dependency. I've added a single patch to
use the git version in the main workspace `Cargo.toml`; when the changes
we need are published, the patch can be removed.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

None yet

2 participants