Skip to content

port drain to std::future#474

Closed
hawkw wants to merge 4 commits intomaster-tokio-0.2from
eliza/0.2-drain
Closed

port drain to std::future#474
hawkw wants to merge 4 commits intomaster-tokio-0.2from
eliza/0.2-drain

Conversation

@hawkw
Copy link
Contributor

@hawkw hawkw commented Apr 13, 2020

This updates the drain crate to use std::future.

Note that because Tokio 0.2's spawn returns a JoinHandle that may
be used to await a task's completion, we can probably simplify the
"waiting for tasks to drain" logic a bit. I didn't do that because I wanted
to do as close to a strict translation as possible, without adding possible
changes in behavior. Also, we are currently still using the compat runtime's
0.1-style spawning.

hawkw added 3 commits April 15, 2020 14:08
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw force-pushed the eliza/0.2-drain branch from c91ea11 to ee9722f Compare April 15, 2020 21:08
@hawkw hawkw changed the base branch from eliza/0.2-update to master-tokio-0.2 April 15, 2020 21:09
@hawkw hawkw marked this pull request as ready for review April 15, 2020 21:10
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

I really like the with simplification. a few questoins

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested review from a team and olix0r April 17, 2020 20:22
hawkw added a commit that referenced this pull request Apr 22, 2020
This branch updates the core proxy components to use `std::future` and
`tower` 0.3. This includes most of the `app` crate and its components,
the `transport` crate, and some miscellaneous stuff in `proxy` that was
necessary for everything else to work.

I know this is a huge PR, but unfortunately, the coupling between these
crates made it kind of necessary. In theory, the proxy's constituent
crates are fairly loosely coupled, and in *most* cases, we don't need to
make this kind of cross-cutting change, but using Tower traits as an
interface is a big part of *how* we are able to implement this loose
coupling. Given the constraint that the `master-tokio-0.2` branch should
always compile, I don't think it's possible to do the `std::future`
update without a branch like this. 

Fortunately, although large, the change here is mostly pretty
mechanical. There's a lot of udpdating `Service` implementations to the
new Tower 0.3 version of the trait, including some slight tweaks to
future implementations to work with stack pinning. Some code (such as
tap and the controller client) had to be commented out to avoid having
to update additional proxy subsystems end-to-end in this branch; we will
update those components in follow-up PRs.

I haven't updated from tokio 0.1 async io to tokio 0.2 async IO; that
will be a later pass. This is because having both `hyper` 0.12 and
`tokio` 0.2 with the "net" feature in the dependency graph at the same
time pulls in incompatible `iovec` versions and is _very_ hard to
untangle. `hyper` and the io types (the `AsyncRead`/`Write` traits,
sockets, etc) will have to move together.

Once this PR lands and the new Tower version is wired all the way
through the core of the proxy, it should be pretty easy to update the
rest of the crates in separate, independent PRs. We can essentially go
through the stack layer-by-layer, and update each component. Once that's
done, then we can circle back and update Hyper and the IO traits to the
Tokio 0.2 versions.

Closes #474 (it's contained in this branch and can't be merged 
independently without breaking stuff)

Signed-off-by: Eliza Weisman <[email protected]>
@kleimkuhler
Copy link
Contributor

@hawkw This can be closed now correct?

@hawkw
Copy link
Contributor Author

hawkw commented Apr 22, 2020

@kleimkuhler yup, I thought the Closes #474 in that PR would do that?

@hawkw hawkw closed this Apr 22, 2020
@kleimkuhler
Copy link
Contributor

I think that may just be for closing issues

@olix0r olix0r deleted the eliza/0.2-drain branch May 25, 2021 15:46
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.

3 participants