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

Stop ignoring semaphore errors in tower-batch #1669

Closed
teor2345 opened this issue Feb 3, 2021 · 0 comments · Fixed by #1692
Closed

Stop ignoring semaphore errors in tower-batch #1669

teor2345 opened this issue Feb 3, 2021 · 0 comments · Fixed by #1692
Assignees
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug
Milestone

Comments

@teor2345
Copy link
Contributor

teor2345 commented Feb 3, 2021

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

tower-batch ignores semaphore errors, but tower::Buffer handles them:
https://github.com/ZcashFoundation/zebra/blob/main/tower-batch/src/service.rs#L97
https://docs.rs/tower/0.4.4/src/tower/buffer/service.rs.html#124

Describe the solution you'd like

We should stop returning Poll::Ready(Ok(())) when the semaphore errors.

Instead, we should return the semaphore errors, mapping them as needed:

        self.semaphore
            .poll_acquire(cx)
            .map_err(|_| self.get_worker_error())

Describe alternatives you've considered

Do nothing: the code hides any semaphore errors, but continues to fail internally.

Additional context

These hidden failures could be causing hangs, so this issue is a high priority.

This issue was discovered during the review in #1593. It could be a cause of the hangs or CPU usage in #1435 or #1634, so it's a high priority.

@teor2345 teor2345 added C-bug Category: This is a bug E-easy A-rust Area: Updates to Rust code S-needs-triage Status: A bug report needs triage labels Feb 3, 2021
@teor2345 teor2345 added this to No Estimate in Effort Affinity Grouping via automation Feb 3, 2021
@teor2345 teor2345 added this to To Do in 🦓 via automation Feb 3, 2021
@teor2345 teor2345 moved this from No Estimate to XS - 2 in Effort Affinity Grouping Feb 3, 2021
@teor2345 teor2345 added this to the 2021 Sprint 2 milestone Feb 3, 2021
🦓 automation moved this from To Do to Done Feb 5, 2021
Effort Affinity Grouping automation moved this from XS - 2 to Done Feb 5, 2021
@mpguerra mpguerra reopened this Feb 5, 2021
🦓 automation moved this from Done to In progress Feb 5, 2021
@mpguerra mpguerra closed this as completed Feb 5, 2021
🦓 automation moved this from In progress to Done Feb 5, 2021
@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-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants