util: fix oneshot dropping pending services immediately#447
Merged
Conversation
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
jonhoo
approved these changes
Apr 23, 2020
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 <[email protected]>
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 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Commit #330 introduced a regression when porting
tower-util::Oneshotfrom
futures0.1 tostd::future. The intended behavior is that aoneshot future should repeatedly call
poll_readyon the oneshottedservice 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
Optionis used to store therequest while waiting for the service to become ready, so that it can be
taken and moved into the service'scall. However, theOptioncontains both the request and the service itself, and is taken the
first time the service is polled.
futures::ready!is then used whenpolling 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 ispolled again, it will panic.
Solution
This commit changes the
Oneshotfuture so that only the request livesin the
Option, and it is only taken when the service is called, ratherthan 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.