Skip to content

Fix off-by-one error in check_all's capacity check #12

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

Merged
merged 2 commits into from
Feb 2, 2020

Conversation

antifuchs
Copy link
Collaborator

This fixes #11:

I'd missed that the "weight" variable is additional weight that gets
added to the first cell, so check_all (when given one more element
than fit in the burst capacity) would return a "denied" result instead
of an "insufficient capacity" result - effectively preventing futures
from ever completing.

Now, we treat additional weight as what it is:

  • rename "weight" to "additional_weight" to make it clear what is
    going on
  • Add a comment over the capacity check, clarifying the off-by-one
    error potential
  • Add a test about for more capacity-excession scenarios, including
    the off-by-one, to prevent regressions.

Thanks to @jean-airoldie for discovering the bug & reporting it!

@antifuchs antifuchs force-pushed the fix-off-by-one-in-all branch from 4173a29 to ccea374 Compare February 2, 2020 00:47
@antifuchs
Copy link
Collaborator Author

Hrm, for some reason the bench task on circleci is failing to run to completion (or even start - it fails to compile criterion): I tried setting the timeout to 20min, but it's still exceeding that timeout. I'm guessing this is either circleci or the rust version that came in docker images is broken atm... we can retry tomorrow.

This fixes #11:

I'd missed that the "weight" variable is _additional weight_ that gets
added to the first cell, so check_all (when given one more element
than fit in the burst capacity) would return a "denied" result instead
of an "insufficient capacity" result - effectively preventing futures
from ever completing.

Now, we treat additional weight as what it is:
* rename "weight" to "additional_weight" to make it clear what is
  going on
* Add a comment over the capacity check, clarifying the off-by-one
  error potential
* Add a test about for more capacity-excession scenarios, including
  the off-by-one, to prevent regressions.
@antifuchs antifuchs force-pushed the fix-off-by-one-in-all branch from ccea374 to 45bcd48 Compare February 2, 2020 14:58
Maybe this will succeed to build now (but I should probably just turn
them off).
@antifuchs antifuchs force-pushed the fix-off-by-one-in-all branch from d46645b to 3bf4b4f Compare February 2, 2020 15:29
@antifuchs
Copy link
Collaborator Author

bors r+

(I had to bump the bench version to nightly, but that at least can compile the criterion crate. Reported as bheisler/criterion.rs#377).

bors bot added a commit that referenced this pull request Feb 2, 2020
12: Fix off-by-one error in check_all's capacity check r=antifuchs a=antifuchs

This fixes #11:

I'd missed that the "weight" variable is _additional weight_ that gets
added to the first cell, so check_all (when given one more element
than fit in the burst capacity) would return a "denied" result instead
of an "insufficient capacity" result - effectively preventing futures
from ever completing.

Now, we treat additional weight as what it is:
* rename "weight" to "additional_weight" to make it clear what is
  going on
* Add a comment over the capacity check, clarifying the off-by-one
  error potential
* Add a test about for more capacity-excession scenarios, including
  the off-by-one, to prevent regressions.

Thanks to @jean-airoldie for discovering the bug & reporting it!

Co-authored-by: Andreas Fuchs <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 2, 2020

Build succeeded

@bors bors bot merged commit 3bf4b4f into master Feb 2, 2020
@bors bors bot deleted the fix-off-by-one-in-all branch February 2, 2020 15:45
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.

DirectRaterLimiter::check_all does not error on exceeded cap
1 participant