Added support and test for async #300
Conversation
There was a problem hiding this comment.
Hm, I have a few comments, but I'm not actually part of this project, and I don't have a ton of confidence in my review, but I thought I'd chime in, in case it helps.
One big picture comment though is that I think the duplication can be eliminated pretty easily?
There are only three spots where we check/increment/set the cache. Why not just do an if/else around each of those. You need to do that anyway, since not every cache has async, so you could do:
if is_async and hasattr(cache, "aadd"):
await do_async_cache_thing()
else:
do_regular_cache_thing()
Could that work?
django_ratelimit/core.py
Outdated
| # python3-memcached will throw a ValueError if the server is | ||
| # unavailable or (somehow) the key doesn't exist. redis, on the | ||
| # other hand, simply returns None. | ||
| count = cache.incr(cache_key) |
There was a problem hiding this comment.
Yes, but there is no aincr that i've seen
There was a problem hiding this comment.
Looks like aincr and adecr have been around since ~4.0 but the implementation isn't atomic, even in caches that should support atomic incr/decr. That's going to be a challenge, even if it gets fixed. I think this is the best choice for now, even if it
There was a problem hiding this comment.
if they are around now then i could add the if statement to the call as well. That will let us support them if the implementation becomes atomic in the future.
django_ratelimit/core.py
Outdated
| # python3-memcached will throw a ValueError if the server is | ||
| # unavailable or (somehow) the key doesn't exist. redis, on the | ||
| # other hand, simply returns None. | ||
| count = cache.incr(cache_key) |
There was a problem hiding this comment.
Yes, but there is no aincr that i've seen
|
This looks fine to me. I'd love to have this merged and released so we can add ratelimits back on our views. |
|
@mlissner Anything left for this to be merged? |
|
@daniel-brenot I'm not a maintainer, I think we need @jsocol to merge, if they're satisfied with this... |
There was a problem hiding this comment.
Looks like Django 3.2 did have some failures that'll prevent merging this exactly as-is.
Looking through the Django cache source, I wonder if there might be a simpler and less duplicative option, which is something like:
async def aratelimit(request, *args, *kwargs):
old_limited = getattr(request, 'limited', False)
ratelimited = sync_to_async(is_ratelimited, ...)
# ... etc ...and since that doesn't rely on the a* methods from the cache, it sidesteps the issue with aincr not being atomic
django_ratelimit/core.py
Outdated
| # python3-memcached will throw a ValueError if the server is | ||
| # unavailable or (somehow) the key doesn't exist. redis, on the | ||
| # other hand, simply returns None. | ||
| count = cache.incr(cache_key) |
There was a problem hiding this comment.
Looks like aincr and adecr have been around since ~4.0 but the implementation isn't atomic, even in caches that should support atomic incr/decr. That's going to be a challenge, even if it gets fixed. I think this is the best choice for now, even if it
| if is_async: | ||
| async def inner(): | ||
| try: | ||
| # Some caches don't have an async implementation |
There was a problem hiding this comment.
It looks like the aadd methods are defined in BaseCache (see the next comment down for the link) so in theory any cache should have access to them
There was a problem hiding this comment.
I've tried it before, and if someone is using an old cache this can fail. This is just to support caches that may not define the cache interface with those methods. Test implementations can also be missing this.
There was a problem hiding this comment.
Can you clarify what you mean by "old cache"? Is it an implementation that doesn't inherit from BaseCache?
| def ratelimit(group=None, key=None, rate=None, method=ALL, block=True): | ||
| def decorator(fn): | ||
| @wraps(fn) | ||
| def _wrapped(request, *args, **kw): | ||
| old_limited = getattr(request, 'limited', False) | ||
| ratelimited = is_ratelimited(request=request, group=group, fn=fn, | ||
| key=key, rate=rate, method=method, | ||
| increment=True) | ||
| request.limited = ratelimited or old_limited | ||
| if ratelimited and block: | ||
| cls = getattr( | ||
| settings, 'RATELIMIT_EXCEPTION_CLASS', Ratelimited) | ||
| raise (import_string(cls) if isinstance(cls, str) else cls)() | ||
| return fn(request, *args, **kw) | ||
| if iscoroutinefunction(fn): |
There was a problem hiding this comment.
Not something to change here, but I wonder if having a second aratelimit decorator might be simpler / clearer. I know we've only got 3 months of 3.2 official support left, but I think it might make it easier to say "aratelimit requires Django >= 4". "Explicit is better than implicit," after all
There was a problem hiding this comment.
Async is barely complete even in Django 5.0, so I doubt anybody would care if 3.2 doesn't support the async decorator. If you're doing async work, you're probably running the latest version of Django.
|
Thank you for the comments @jsocol! I truly appreciate your benevolent maintenance of this library. |
Isn't it specifically Django 3.2 on Python 3.7 that has the failure? The tests running in Python 3.8+ seem to pass? 🤔 |
I think we should use the async versions where possible, even if they aren't atomic yet. If they become atomic in the future, then the work is already done. As for using sync to async, i'd prefer to avoid that if possible. It may be an easier solution, but if any of the async methods become better in the future we lose out on the performance improvement. Given that this may be used on methods called often, this could make a substantial difference in performance to a server. |
An atomic increment operation is, unfortunately, very much a requirement for this library to work correctly. (See the discussion on #255 regarding the database cache backend.) Without either atomic increment or check-and-set, requests will be under-counted. Two requests that read the same current value will both set the same "next" value.
100% @benjaoming—I was being a little lazy, saw the Dj3.2-Py3.7 failure and didn't keep searching. Looks like just this combo and some minor linting issues. |
|
I'm pretty lazy here, but if it's just...
...I still think the thing to do is just say async isn't supported on django 3.2 and block off the code path. 3.2 didn't have much async support at all, and it's only got three months of support left despite being a LTS. (Sorry to not have more meaningful code comments. :) ) |
Added an example test and general async support. Should work well