Added support and test for async

[daniel-brenot]

Added an example test and general async support. Should work well

[mlissner]

This looks fine to me. I'd love to have this merged and released so we can add ratelimits back on our views.

[daniel-brenot]

@mlissner Anything left for this to be merged?

[mlissner]

@daniel-brenot I'm not a maintainer, I think we need @jsocol to merge, if they're satisfied with this...

[mlissner]

Thank you for the comments @jsocol! I truly appreciate your benevolent maintenance of this library.

[benjaoming]

Looks like Django 3.2 did have some failures that'll prevent merging this exactly as-is.

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? :thinking:

[daniel-brenot]

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

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.

[jsocol]

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.

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.

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? 🤔

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.

[mlissner]

I'm pretty lazy here, but if it's just...

Django 3.2 on Python 3.7 that has the failure

...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. :) )