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

DirectRaterLimiter::check_all does not error on exceeded cap #11

Closed
jean-airoldie opened this issue Feb 1, 2020 · 2 comments · Fixed by #12
Closed

DirectRaterLimiter::check_all does not error on exceeded cap #11

jean-airoldie opened this issue Feb 1, 2020 · 2 comments · Fixed by #12

Comments

@jean-airoldie
Copy link
Contributor

jean-airoldie commented Feb 1, 2020

If I correctly understand the DirectRateLimiter::check_all doc, it should error when a n greater than the initial max_burst when creating the Quota is passed. However it is not the case:

#[test]
fn errors_on_exceeded_cap() {
    let lim = RateLimiter::direct(Quota::per_second(nonzero!(10u32)));

    // This does not error even though we exceed the capacity.
    block_on(lim.check(nonzero!(11u32))).unwrap_err();
}
@antifuchs
Copy link
Collaborator

Thanks for reporting this issue (and for including a test case - that's really helpful)! I had to adjust it a bit so it would compile & then error:

#[test]
fn errors_on_exceeded_cap() {
    let lim = RateLimiter::direct(Quota::per_second(nonzero!(10u32)));

    // This does not error even though we exceed the capacity.
    assert_eq!(
        Err(NegativeMultiDecision::InsufficientCapacity(10)),
        lim.check_all(nonzero!(11u32))
    );
}

I guess we're seeing an off-by-one error somewhere!

@antifuchs
Copy link
Collaborator

Yeppppp, this is definitely an off-by-one error in the weight > tau check in test_n_all_and_update. I'll think about this some more.

antifuchs added a commit that referenced this issue Feb 1, 2020
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.
bors bot added a commit that referenced this issue 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 <asf@boinkor.net>
@bors bors bot closed this as completed in 45bcd48 Feb 2, 2020
@bors bors bot closed this as completed in #12 Feb 2, 2020
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 a pull request may close this issue.

2 participants