alisaifee / limits

Rate limiting using various strategies and storage backends such as redis & memcached
https://limits.readthedocs.org
MIT License
423 stars 58 forks source link

Issue with implementing limits in fastapi #166

Closed anandtripathi5 closed 1 year ago

anandtripathi5 commented 1 year ago

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

anandtripathi5 commented 1 year ago

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?

alisaifee commented 1 year ago

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.

anandtripathi5 commented 1 year ago

Totally makes sense

alisaifee commented 1 year ago

@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.

alisaifee commented 1 year ago

Resolved in 3.4.0