arnetheduck
commented
3 months ago Given the fact that lastUpdate is initialized at creation of TokenBucket instance
indeed, the bucket could start empty and with a 0 timestamp - this situation should clear up after the first use though so I don't think it affects the "average".
Looking at the code, some other small issues stand out:
- partially full buffers don't contribute to
tryConsumebut get overwritten on replenish - queued consumptions look a bit wonky for the same reason, ie the logic for what happens to lastUpdate / budget after a queued consume has been satisfied could be reviewed
I suspect a bug with the TokenBucket update calculation. I think due to the lastUpdate field is updated wrong thus leads false calculation in update method and result in refilling the budget even if within time frame not elapsed.
Given the fact that lastUpdate is initialized at creation of TokenBucket instance and it can be far in time from the first consume event. Through the calculations of fill percentage will give wrong result if we compare current update time to the time of lastUpdate. In my understanding lastUpdate shall be refreshed at the time when the budget is full and the first tokens are to be consumed.
TokenBucket.init (3, 500.millis)-> 5 sec -> consume, consume, consume (within 50ms), consume last consume should fail but seems update refills the budget and sets lastUpdated ahead with 5sec.
https://github.com/status-im/nim-chronos/blob/672db137b7cad9b384b8f4fb551fb6bbeaabfe1b/chronos/ratelimit.nim#L40
Some background, I started using TokenBucket as a request rate limiter in Waku's non relay protocols. It should add cap on request count inside a certain amount of time. So as I would use it is the within a certain time period max budgetCap number of request is allowed, when the period is over it shall refill the budget.
Now I'm not pretty sure I expect something different from TokenBucket as it is for or it is a bug. Please let me know how do you see!
Update: I checked the tests and found this one suspicious: https://github.com/status-im/nim-chronos/blob/672db137b7cad9b384b8f4fb551fb6bbeaabfe1b/tests/testratelimit.nim#L119
I think it can be reproduced what's happening in CI if you add a 1ms wait after creating the TokenBucket https://github.com/status-im/nim-chronos/blob/672db137b7cad9b384b8f4fb551fb6bbeaabfe1b/tests/testratelimit.nim#L125 Than the replenish calculation will go wrong as descibed above, similar to my case.
CC: @arnetheduck, @cheatfate