Fix poll_ready usage for the Inbound service #1620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teor2345
added
C-bug
teor2345
force-pushed
the
poll-ready-inbound
branch
from
22cf22a to
61849c7
Compare
2 tasks
yaahc
reviewed
Jan 22, 2021
teor2345
commented
Jan 22, 2021
teor2345
commented
Jan 22, 2021
teor2345
commented
Jan 22, 2021
teor2345
commented
Jan 22, 2021
teor2345
commented
Jan 22, 2021
This was referenced Jan 25, 2021
teor2345
force-pushed
the
poll-ready-inbound
branch
3 times, most recently
from
ae7cbdc to
f6a8677
Compare
Uses the `ServiceExt::oneshot` design pattern from ZcashFoundation#1593.
Use `ServiceExt::oneshot` to perform state requests. Explain that `ServiceExt::call_all` calls `poll_ready` internally. Document a state service invariant imposed by `ServiceExt::call_all`.
ServiceExt::call_all leaks Tower::Buffer reservations, so we can't use it in Zebra. Instead, use a loop in the returned future. See ZcashFoundation#1593 for details.
And log an info-level message as a diagnostic, in case setup takes a long time.
This change encodes a bunch of invariants in the type system, and adds explicit failure states for: * a closed oneshot, * bugs in the initialization code.
teor2345
force-pushed
the
poll-ready-inbound
branch
from
f6a8677 to
df39910
Compare
1 task
teor2345
commented
Jan 27, 2021
teor2345
commented
Jan 27, 2021
1 task
1 task
9 tasks
9 tasks
3 tasks
Rather than having them default to `Ok(())`, which is incorrect for some error handlers.
|
@yaahc @oxarbitrage I think we're pretty close to the final version of this fix |
|
The windows failure is an unrelated linker error #1651 |
|
I'm re-running CI to test that windows passes after the latest fixes. |
|
You need to rebase the PR this the windows test to pass. |
GitHub runs CI on a merge commit with the latest |
yaahc
approved these changes
Feb 2, 2021
|
The macOS testnet failure is a known issue. |
This was referenced Feb 3, 2021
teor2345
commented
Feb 15, 2021
Comment on lines
+248
to
+251
| // Correctness: | ||
| // | ||
| // We can't use `call_all` here, because it leaks buffer slots: | ||
| // https://github.com/tower-rs/tower/blob/master/tower/src/util/call_all/common.rs#L112 |
There was a problem hiding this comment.
This comment is true, but for slightly different reasons:
call_allcan hold one buffer slot per clone for a long time- we're not sure when the returned future will complete
- we don't limit how many returned futures can be concurrently running
So we still can't use call_all.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
Inboundservice misusespoll_ready, filling up buffers, and potentially causing hangs.See #1593 for a general explanation.
Solution
Make sure each
poll_readyis followed by acall:ServiceExt::oneshotpattern from Make sure Zebra uses poll_ready and Buffer reservations correctly #1593ServiceExt::call_allusespoll_readyinternallypoll_readyconstraint due to a bug inServiceExt::call_allThe code in this pull request has:
Review
@yaahc and @oxarbitrage might want to look at this PR to see some
poll_readyexample code.Related Issues
Solves some of the hangs from #1435
Follow Up Work
Do the rest of #1593