Closed AaronErhardt closed 1 year ago
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 (:
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: https://github.com/AaronErhardt/actix-governor/pull/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: