-
Notifications
You must be signed in to change notification settings - Fork 45
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
Possible regression from 0.4 to 0.5 #157
Comments
Hrmmmmm! Thanks for the report. This is pretty concerning - I hoped that this time, I'd have gotten the clock behavior right so tests reflect actual reality. I'll dig, but can't promise that I'll get it done before the holidays. /: |
Actually, this seems to be easier than I thought: The tests involving futures&"real" clocks end up failing when the quota reporting is fixed, and exhibit very weird & flakey behavior. I suspect that quanta (the clock provider) is being a bit odd (it used to inject "calibration" runs the first time you'd measure, which added 50 or so millis of jitter to every test run). |
Think I've got a fix for this, in #159! Hope tests pass this time around (: |
159: Correctly report quota in StateInformationMiddleware r=antifuchs a=antifuchs This fix was previously prevented from passing tests by flakey quanta clock tests. I believe by lengthening the intervals and reducing the quota (thereby increasing the "time value" of each element), we can de-flake these tests somewhat, which lets this fix correctly pass. Now, the problem was that the correct quota was reported only if a fake clock was used (i.e. if it remains at the zero nanosecond) - instead, we correctly compute the remaining capacity in all cases now. This also extends the "remaining capacity" test by a few cases including a real clock. Fixes #157 Co-authored-by: Andreas Fuchs <asf@boinkor.net>
Released in 0.5.1, which should hit docs.rs and crates.io in moments! |
Awesome! Actix-governor no longer lags behind the official governor release: AaronErhardt/actix-governor#37 Thanks a lot for this quick fix, Andreas! |
Thank you for the super detailed & effective report! |
Maintainer of actix-governor here.
After upgrading actix-governor's version of governor, our test cases started to tail.
It doesn't look like the rate-limiting itself is broken, but the reporting of the remaining burst size.
While looking through the changes I noticed that this test was added:
https://github.com/antifuchs/governor/blob/b8d99cf6b01c1969bce2e3497986b2ae59d33ac4/governor/tests/middleware.rs#L73-L102
According to the test, everything seemed to work as expected: After one check, the remaining burst size goes down by one.
Yet in our tests it went down from two to zero in just one step.
After some testing I found that using
FakeRelativeClock
made a difference.Running with the fake clock works, but with a real clock it doesn't anymore.
This test case shows this behavior:
The text was updated successfully, but these errors were encountered: