Skip to content

put back transport metrics#530

Merged
hawkw merged 1 commit intomaster-tokio-0.2from
eliza/0.2-trans-metrics
May 27, 2020
Merged

put back transport metrics#530
hawkw merged 1 commit intomaster-tokio-0.2from
eliza/0.2-trans-metrics

Conversation

@hawkw
Copy link
Contributor

@hawkw hawkw commented May 26, 2020

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.

Depends on #526 and will be rebased onto master-tokio-0.2 when
that branch merges.

Signed-off-by: Eliza Weisman [email protected]

@hawkw hawkw requested a review from a team May 26, 2020 18:20
@hawkw hawkw self-assigned this May 26, 2020
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]>
@hawkw hawkw force-pushed the eliza/0.2-trans-metrics branch from d971fbe to be666a4 Compare May 26, 2020 19:48
@hawkw hawkw changed the base branch from eliza/0.2-outbound to master-tokio-0.2 May 26, 2020 19:48
@hawkw hawkw requested a review from olix0r May 26, 2020 19:48
@hawkw hawkw requested a review from a team May 27, 2020 20:45
@hawkw hawkw merged commit 3cb4545 into master-tokio-0.2 May 27, 2020
@hawkw hawkw deleted the eliza/0.2-trans-metrics branch May 27, 2020 21:23
hawkw added a commit that referenced this pull request May 27, 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.

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.

3 participants