Cache burst issue

[srikrsna]

Hi,

We were testing this library for use as we already use Chi and found an issue with the current implementation.

Currently's it first checks the status and then increments. This will create an error in the following scenario which can be replicated with a unit test.

If the request limit is say 25/sec If I get 50 requests simultaneously in the first second then it will let them through as for every request the initial value will be 0.

Instead, if we can have a way in which the current window can be incremented and retrieved along with the previous window then this can be fixed.

type LimitCounter interface {
    // IncrAndGet is an atomic operation
    IncrAndGet(key string, inc time.Time, get time.Time) (inc int64, get int64, err error)
}

We wanted to use Redis and the example which I stated is doable using INCR command.

Please let me know your thoughts.

[maxzaleski]

Hi,

Can anyone follow up on this?

Cheers

[yaronius]

@pkieltyka is this repo abandoned?

[pkieltyka]

Nope. This package is solid and I use it in prod in many places

[pkieltyka]

Good idea, feel free to PR this. Or if you provide the breaking test code, I'll write the solution in httprate

[pkieltyka]

solved in #20

however, I think exploring an iteration on LimitCounter interface is a good idea too (as mentioned in this ticket)

[VojtechVitek]

IncrAndGet() would increase the counter at all times, even if we respond with HTTP 429 Too many requests.

We don't want that. We need to increase the counter only if we allow the request to pass through.

We'd need an atomic method that increments the counter only within a given limit.

IncrementWithLimit(key string, amount int, limit int) (int, error)

value = GET key

if value + increment <= limit {
  INCRBY key increment
  EXPIRE key
}

In Redis, this could be done only with LUA script.

However, it's even more complicated -- we need to fetch two keys (previous window + current window), calculate the rate and then conditionally INCRBY within a limit. Such method would have to re-implement too much logic:

AtomicOperation(key string, currentWindow, previousWindow time.Time, amount int, limit int) (int, bool, error)

I don't think this is worth it.

I think it's fine to keep the existing interface as is.