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

Fix poll_ready usage for the script::Verifier service #1681

Closed
3 tasks
teor2345 opened this issue Feb 3, 2021 · 2 comments
Closed
3 tasks

Fix poll_ready usage for the script::Verifier service #1681

teor2345 opened this issue Feb 3, 2021 · 2 comments
Labels
A-docs Area: Documentation A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup

Comments

@teor2345
Copy link
Contributor

teor2345 commented Feb 3, 2021

This ticket was obsoleted by the fixes to the underlying issue in tower::Buffer, see #1593 for details.

Is your feature request related to a problem? Please describe.

The script::Verifier service uses poll_ready correctly, but in a risky way. (See #1593.)

The code works right now. But if the StateService is changed, the script::Verifier could fill up Buffers or Batches, potentially causing hangs.

Describe the solution you'd like

Make sure each poll_ready is followed by a call:

  • Always return Poll::Ready(Ok(())) from script::Verifier::poll_ready
    • you can delete the current implementation - it's correct but risky
  • Use ServiceExt::oneshot to call the StateService
  • Document the poll_ready/call invariants to avoid future changes causing hangs

The documentation should look something like:

// Correctness:
//
// We avoid calling `poll_ready` on state service here, because that makes it easier to leak buffer slots.
// See #1593 for details.

See #1593 for a detailed background explanation.

Describe alternatives you've considered

Don't make any changes. Future changes might cause Zebra to hang.

Follow Up

Handle backpressure correctly (#1618)

Original Issues

Design: Make sure all poll_ready reservations are actually used in a call #1593
Example: Fix poll_ready usage for the Inbound service #1620

@teor2345 teor2345 added C-bug Category: This is a bug A-docs Area: Documentation A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup S-needs-triage Status: A bug report needs triage P-High labels Feb 3, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Feb 3, 2021

@oxarbitrage you might want to work on these script::Verifier fixes after you've finished all your other high-priority tasks.

Or @yaahc you might want to work on this, if you finish the proof verifier before @oxarbitrage has finished his other work.

@teor2345
Copy link
Contributor Author

This ticket was obsoleted by the fixes to the underlying issue in tower::Buffer, see #1593 for details.

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup
Projects
None yet
Development

No branches or pull requests

2 participants