Threading based lock will not work on multi-machine system about limits HOT 6 CLOSED

Hi @twolfson https://github.com/alisaifee/limits/blob/1.0.6/limits/storage.py#L249 refers to https://github.com/andymccurdy/redis-py/blob/master/redis/lock.py#L9 which uses redis for its locking.

from limits.

Ah, sorry. I must have gotten my thoughts confused. Thanks for clarifying.

from limits.

On a tangent from this, is the locking mechanism required? In case of a denial of service attack, then that would require requests to be held open longer which is more likely to take the service down due to running out of connections.

from limits.

The lock is only used in the moving window implementation and isn't strictly required as in the worst case only one extra request would get through. At the very least I think a reasonably small timeout when trying to acquire the lock could help. Failure to acquire the lock within the timeout could result in the rate limit being marked as 'hit' as a client is bursting at a rate high enough to hit a lock is probably not up to any good. What do you think?

from limits.

I think both options have their benefits. It most likely boils down to whether the developer wants to be as performant as possible or as accurate as possible.

Marking the lock acquisition as a failure might be interesting. How would it work with a non-responsive Redis instance? In those cases, it might be better to 500 rather than 429.

from limits.

@twolfson: I've added a lock timeout in b6b755d
If the lock can't be acquired it will raise a LockError which when used with Flask-Limiter will currently result in a 500 error. I'll further add new options in Flask-Limiter to configure what to do when an error is encountered when attempting to rate limit a route.

from limits.