alisaifee/limits

Issue with implementing limits in fastapi

anandtripathi5 opened this issue · 5 comments

I'm implementing limits in fastapi to have more control over the ratelimiting logic that cannot be done by other libraries. I have the below implementation

limiter = FixedWindowElasticExpiryRateLimiter(storage_from_string("async+redis://localhost:6379"))

def app_rate_limit(func):
    @wraps(func)
    async def wrapper(*args, **kwargs):
            is_allowed = await limiter.hit(parse('1/second'), "random-key")
            if not is_allowed:
                raise HTTPException(status_code=429, detail="Too many requests")
        return await func(request, *args, db=db, **kwargs)
    return wrapper

And in the fastapi route I'm attaching the above decorator

@router.post(...)
@app_rate_limit
def route(...)
    pass

its giving the below error

  File "/Users/anandtripathi/.virtualenvs/verification-backend-CPjzdNsq-py3.8/lib/python3.8/site-packages/limits/aio/strategies.py", line 196, in hit
    amount = await self.storage.incr(
ReferenceError: weakly-referenced object no longer exists

I checked inside the code of limits it seems to be coming from storage initialization but not sure what I'm doing wrong

Aah its its seems from inside the code the storage is self.storage: Storage = weakref.proxy(storage) weakref proxy. So it will not work with below code

limiter = FixedWindowElasticExpiryRateLimiter(storage_from_string("async+redis://localhost:6379"))

But instead I have to put the storage in a variable so that the object will not be collected by garbage collector once its out of scope. So I have to change it to below code

redis = storage_from_string("async+redis://localhost:6379")
limiter = FixedWindowElasticExpiryRateLimiter(redis)

Can we include this somewhere in the docs?

Yeap you got it! Looks like that code has been mostly the same for about 10 years (ref and I can't actually reason why the weak reference is even there. It might be simpler to just change the code to use a strong reference.

Totally makes sense

@anandtripathi5 thanks again for raising this. I've removed the weak reference in 3.4.0 so if you upgrade you should be able to do without the workaround.

Resolved in 3.4.0