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 tower-util and tower to std::future #330

Merged
merged 4 commits into from
Sep 10, 2019

Conversation

jonhoo
Copy link
Collaborator

@jonhoo jonhoo commented Sep 9, 2019

This bumps tower-util and tower to 0.3.0-alpha.1.

One noteworthy change here is that CallAll now also has a take_service method that can be used through a Pin. Not sure whether it is necessary, but it was handy for tests.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jonhoo Jon Gjengset
@jonhoo jonhoo requested a review from LucioFranco September 9, 2019 21:56
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM no blockers!

Verified

This commit was signed with the committer’s verified signature. The key has expired.
jonhoo Jon Gjengset
@jonhoo jonhoo merged commit 0802ca2 into v0.3.x Sep 10, 2019
@jonhoo jonhoo deleted the jonhoo/update-util-and-root branch September 11, 2019 15:10
hawkw added a commit that referenced this pull request Apr 23, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
## 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 the #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
`take`n 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>
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