Add async support?

[mlissner]

We are using this package with great success on our website, but as we move to the async model, we're finding that it doesn't work.

If we added async support to this package, would that be welcomed by the maintainers? If so, we can probably take a stab at doing so. If not — and that's perfectly OK! — we may consider making a small fork with that functionality and calling it django-aratelimit, or something like that. (Haven't given this too much thought, since it'd be much better to add the functionality here!)

Any thoughts or suggestions about doing this before we go further down this road?

Thank you for the great package.

[jsocol]

Hi @mlissner, thanks for raising this!

I'm definitely open to adding async support here. I'd love to know what isn't working—I'm guessing decorating an async function, but maybe there are other things?—and how you think the API might need to change?

[mlissner]

I'm not an expert here, but yes, decorating an async function is where we ran into trouble.

There's some prior art in https://pypi.org/project/django-fast-ratelimit/, where their decorator can work in both sync or async, but I haven't looked to see how that works. That does feel like the ideal solution though.

A less elegant solution might be to have an aratelimit decorator that's compatible with the regular ratelimit decorator.

[jsocol]

Gotcha, thank you! There was a similar problem in the statsd library, and we fixed it by decorating the function differently if it's a coroutine: https://github.com/jsocol/pystatsd/blob/main/statsd/client/timer.py#L24-L45

[benjaoming]

A less elegant solution might be to have an aratelimit decorator that's compatible with the regular ratelimit decorator.

I don't think that's necessarily less elegant, it could be very useful and in any case, it's also very explicit, given that people know the convention :)

Prefixing with an a, is pretty conventional in Django already: https://docs.djangoproject.com/en/4.2/topics/async/

[mlissner]

Yeah, it's not horrible, but it would have been really nice to take a view like this:

@ratelimit
def my_page(...):

And make it async by just doing:

@ratelimit
async def my_page(...)

I think that's pretty cool, though I agree that a-prfixing it isn't the end of the world. :)

[jsocol]

@mlissner I believe we can support that API using the same technique as the pystatsd timer class. But, I think I might need more help understanding exactly how the failure is happening right now.

In #295 I tried adding a test that I thought would fail, but it doesn't—and when I think about it, I understand why it doesn't.

Can you share a little more about the issues you're seeing and the expected/actual behavior?

[mchingotto]

Hi Guys, I hope you are ok.
Maybe something interesting could be to not depend on the request object. When using celery for example, that is a huge issue since request is not serializable, so problems arise when rate-limit is needed in async tasks.
I would appreciate any input if I missing something here.
Thank you,
M

[jsocol]

Hi @mchingotto would you mind opening another issue for that? It's a pretty different situation

[ttys0dev]

In #295 I tried adding a test that I thought would fail, but it doesn't—and when I think about it, I understand why it doesn't.

I modified that test in #298 and got something that fails when expected now by asserting that the function is actually async(which is how django determines how it should be called).

[daniel-brenot]

I've got some code that's been working for us I can put in a pr. It follows how django makes async decorators, so it should generally work alright, but please feel free to double check it.

[daniel-brenot]

#300 Should fix this as long as I can get confirmation from someone else that this works for them too. I'll be testing it on my work project to see if it helped.

[jsocol]

Thanks, folks! Just to make sure I understand what the issues are, let me paraphrase:

• Django uses iscoroutinefunction to determine whether or not to run the view async at all. That means, if we wrap an async view in a sync decorator, the decorator still works (as far as I can tell?) but we lose the benefits of having the async view in the first place.
• When we are wrapping an async view, we should be using the async cache methods aadd and aincr so we're not unnecessarily blocking.

That sound right?

The API in #300 (ais_ratelimited and aget_usage) seem solid, though I want to think a little bit about the structure to see if we can eliminate some of the duplication, or, if not, at least some of the nesting.

[daniel-brenot]

Actually no, the async view does not work if we wrap it in a sync decorator because it doesn't await the view. This is why I did this in the first place as we didn't have a way to use this.

As for the second point, yes. Although it looks like aincr doesn't exist from what I saw? if it does then i'm happy to change that line.

That's fair that you want to eliminate duplication. I tried my hardest to, hence why there is a function that accepts an is_async argument so we don't have so much code reuse. I'm not the biggest fan of the fact that in python we have to have a sync and async version of each function, but it is what it is.

Let me know if there is anything else I can help with. I'm excited to have this merged so our project can use it and we don't have to keep the code locally lol.

[mlissner]

I added some comments on the PR. I hope they're helpful and move this forward a bit.

Also to give examples of how async views decorated with this package fail, I can share this issue: freelawproject/courtlistener#2930

[mlissner]

Hi all, I just wanted to follow up. We've been using the usual ratelimit decorator on our views at work, and it'd really be nice to get an async version put in place. If you use the regular decorator, like we do, it sort of works — whenever the rate limiter is hit, the view crashes. Could be worse, but that sends a Sentry event for us, which isn't great either.

@daniel-brenot are you still using your version from #300 and is it working properly?

If #300 is working for @daniel-brenot for the past month+, maybe we should merge it and give it some more light of day?

[daniel-brenot]

My fork has been updated with the async versions of all the existing tests. If this is satisfactory, i'd recommend a look over

[mlissner]

I just added a comment to your PR, @daniel-brenot, but I hope we can get his out the door soon. Anything my organization can do to help?

[daniel-brenot]

I just added a comment to your PR, @daniel-brenot, but I hope we can get his out the door soon. Anything my organization can do to help?

Nope. The PR is fully tested and just waiting on approval. I don't anticipate any issues with the PR, it passes all of the tests i rewrote for async so it seems to work fine.