Skip to content

Update tower-util and tower to std::future#330

Merged
jonhoo merged 4 commits intov0.3.xfrom
jonhoo/update-util-and-root
Sep 10, 2019
Merged

Update tower-util and tower to std::future#330
jonhoo merged 4 commits intov0.3.xfrom
jonhoo/update-util-and-root

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.

@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!

@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
## 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 <[email protected]>
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.

2 participants