Multiple limit not respected about asgi-ratelimit

[abersheeran]

I think your understanding is correct. I read your test code and it should work like this.

[abersheeran]

If there are any bugs here, PR is welcome. 😀

[euri10]

If there are any bugs here, PR is welcome.

not sure where to look at to be honest, I'd suppose at first sight in the calc_incr_value method ? I see incr_dict has its keys decremented by 1, should there be a global count in case there are multiple limits ?

I also note that the decrease_limit script always returns None when running the test_redis, but I don't understand well the logic behind this.

Would you explain briefly the logic behind, I'd be ok trying to fix this but I'd need some guidance before :)

[abersheeran]

I also note that the decrease_limit script always returns None when running the test_redis, but I don't understand well the logic behind this.

decrease_limit is an atomic operation. It returns True after successfully decrementing all keys by 1, otherwise it returns False. It shouldn't return None in design, can you tell me how to reproduce it to return None?

[euri10]

I just issued a draft PR so that you see it, it's not meant to be a PR but I figured it was quicker than to copy-paste stuff

[abersheeran]

This is so strange that such behavior shouldn't happen. It seems that aredis failed to execute the Lua script but did not throw an exception, only returning None.

[abersheeran]

aredis's author seems to be busy with 996 work. I think it's time to switch to another asynchronous redis driver.

[euri10]

aredis's author seems to be busy with 996 work. I think it's time to switch to another asynchronous redis driver.

I was looking at the landscape the other day and it's a landmine @abersheeran !
What I can say: I use arq, which uses aioredis without sweating, but this is limited knowledge, I'll ask at arq given the paragraph below what they think they'll do:
it seems aioredis went unmaintaned for a while, then back up, then this PR seems to indicate a fork to come to aioredis-py which will be a total rewrite based more on redis-py, and aioredis will likely come back to sleep mode.
Hard to choose

[abersheeran]

I agree. Hard to choose.

Back to asgi-ratelimit. We need to verify my guess: whether the lua script is running normally first. But I'm a bit busy, can you help me verify it?

[euri10]

Ibit busy to, will try asap :)

…

On Wed, Mar 17, 2021 at 10:06 AM Aber ***@***.***> wrote: I agree. Hard to choose. Back to asgi-ratelimit. We need to verify my guess: whether the lua script is running normally first. But I'm a bit busy, can you help me verify it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINSPRG5F6GGDF5EEVDFPDTEBWIZANCNFSM4ZH7ANUA> .

-- benoit barthelet http://pgp.mit.edu/pks/lookup?op=get&search=0xF150E01A72F6D2EE

[euri10]

ok I think your guess is correct, seems like the lua script is the culprit and doesn't work as intended, I base this on implementing a quick and dirty branch based on aioredis and it returns also None every time as well: https://github.com/euri10/asgi-ratelimit/tree/aioredis

now my lua knowledge is very low :) will try to see what goes wrong as well,

[abersheeran]

Thank you for your help! I just need to focus on this lua script next. Maybe I can fix it these days.

[euri10]

from the docs https://redis.io/commands/eval
it seems the return false in the decrease script is the culprit:

Lua boolean false -> Redis Nil bulk reply.

[euri10]

changing the script to

DECREASE_SCRIPT = """
for i, key in ipairs(KEYS) do
    local value = tonumber(redis.call('GET', key))
    if not value or value < 1 then
        return 0
    end
end
for i, key in ipairs(KEYS) do
    redis.call('DECR', key)
end
return 1
"""

solves the None issue, but does not change the outcome of the test, so there's something else going on