Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partly revert "Fix poll_ready usage in ChainVerifier" #1735

Merged
merged 4 commits into from
Feb 20, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Feb 15, 2021

Reverts #1700

This revert is ok, because the underlying bug in tower::Buffer has been fixed. See #1593 for details.

But the code needs a few tweaks to avoid unlikely deadlocks.

The underlying services aren't directly buffered, but they do use some buffered services. We have open tickets for checking their buffer bounds (#1675), and dealing with backpressure more generally (#1618).

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup P-Medium labels Feb 15, 2021
@teor2345 teor2345 added this to the 2021 Sprint 3 milestone Feb 15, 2021
@teor2345 teor2345 requested a review from yaahc February 15, 2021 05:27
@teor2345
Copy link
Contributor Author

I think @yaahc should check this PR out as well. Turns out the revert is non-trivial.

@teor2345
Copy link
Contributor Author

The Windows CI failure should be fixed by #1736.

@teor2345 teor2345 changed the title Revert "Fix poll_ready usage in ChainVerifier" Partly revert "Fix poll_ready usage in ChainVerifier" Feb 15, 2021
@teor2345 teor2345 added I-slow Problems with performance or responsiveness I-hang A Zebra component stops responding to requests labels Feb 16, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Feb 17, 2021

It's possible that the small buffers fixed in this PR could hang

@teor2345 teor2345 requested a review from a team February 19, 2021 22:38
Copy link
Contributor

@yaahc yaahc left a comment

Choose a reason for hiding this comment

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

looks good to me

zebra-consensus/src/chain.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

Hmm seems like CI failed because cargo couldn't download crates. Retrying.

@teor2345 teor2345 removed the request for review from oxarbitrage February 19, 2021 23:20
@teor2345 teor2345 merged commit 3af57ec into main Feb 20, 2021
@teor2345 teor2345 deleted the revert-1700-issue1679 branch February 20, 2021 00:43
@teor2345 teor2345 mentioned this pull request Feb 23, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup I-hang A Zebra component stops responding to requests I-slow Problems with performance or responsiveness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants