Skip to content

update reconnect to std::future#531

Merged
hawkw merged 8 commits intomaster-tokio-0.2from
eliza/0.2-reconnect
May 27, 2020
Merged

update reconnect to std::future#531
hawkw merged 8 commits intomaster-tokio-0.2from
eliza/0.2-reconnect

Conversation

@hawkw
Copy link
Contributor

@hawkw hawkw commented May 26, 2020

This branch updates the reconnect middleware to std::future.
Updating reconnect was pretty straightforward and mechanical, so it
should be easy to review.

Most of the complexity was actually in getting the test
retry_reconnect_errors which exercises this functionality to pass: the
test uses transport metrics to determine when the proxy has actually
accepted a connection from the test client, so that it can advance to
the next state; it was necessary to re-enable transport metrics for this
to work (see PR #530).

Additionally, there was a subtle change in the test-support client's
behavior that needed to be resolved as well: the test relied on
Client::request_async eagerly causing the test client to connect to
the proxy when called, rather than when the returned future is polled.
Switching request_async to an async fn broke this property — the
entire body of the function, even the synchronous parts, was run only
when the async fn future is polled. This means that the connection was
not opened when the test expected it to be, and the subsequent assertion
that it was open always failed.

Therefore, I changed request_async back into a synchronous function
returning impl Future. This is identical at the callsite (it still
returns something that you can .await), but calling the function now
once again eagerly tells the test client background thread to open a
connection. This fixes the test.

Depends on #530.

hawkw added 7 commits May 26, 2020 12:46
turns out these _worked_ but i never actually put them back. whoopsie!

this is necessary for the only integration test that really exercises
reconnects, which relies on these metrics to determine if a connection
exists.

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
This is a fun one: the test `retry_reconnect_errors` relied on
`Client::request_async` *eagerly* causing the test client to connect to
the proxy when called, rather than when the returned future is polled.
Switching `request_async` to an `async fn` broke this property — the
*entire* body of the function, even the synchronous parts, was run only
when the `async fn` future is polled. This means that the connection was
not opened when the test expected it to be, and the subsequent assertion
that it was open always failed.

This commit changes this back into a synchronous function returning
`impl Future`. This is identical at the callsite (it still returns
something that you can `.await`), but calling the function now once
again eagerly tells the test client background thread to open a
connection. This fixes the test.

Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested review from a team and olix0r May 26, 2020 19:53
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw requested a review from a team May 27, 2020 20:44
@hawkw hawkw self-assigned this May 27, 2020
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

👍

@hawkw hawkw merged commit bbcd07a into master-tokio-0.2 May 27, 2020
@olix0r olix0r deleted the eliza/0.2-reconnect branch May 25, 2021 15:49
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