django / channels

Developer-friendly asynchrony for Django
https://channels.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6.08k stars 800 forks source link

Add cancel_callback in dispatch #1948

Open fosterseth opened 1 year ago

fosterseth commented 1 year ago

pairs with this PR https://github.com/django/channels_redis/pull/339

to address a memory leak issue in pubsub backend https://github.com/django/channels_redis/issues/276

Here is the race condition in pubsub.py that leads to the memory leak:

Solution is to set a cancel_callback in the dispatch code that will run when hitting an asyncio.CancelledError. By default this callback will be None, but if the channels_layer has a clean_channel method, it will call that instead (with the channel name as the argument, so we know which channel to cleanup)

TODO: maybe tests? I might need some help here

acu192 commented 1 year ago

Good work on this. These race conditions are certainly hard to think about, but I agree with your analysis.

This solution looks good to me! It is similar what I've wanted to do for a while, but never found the time. Thank you!

I don't have permissions to run workflows in this repo, but I'd certainly recommend it to @carltongibson.

qeternity commented 1 year ago

Looks good to me. Nice job! Left one small comment re: usage of getattr.

carltongibson commented 1 year ago

Hi @fosterseth Thanks for this. (And thanks @acu192 and @qeternity for checking it out.)

Just to let you know it's on my list for 4.1 updates that I'm looking at over the end of year period.

TODO: maybe tests? I might need some help here

Yes. 🤔 So I guess a mocked channel layer, providing the clean channel method, and verifying that it is scheduled and run.

fosterseth commented 1 year ago

thanks for looking over this @carltongibson I'll work on getting a test together

bigfootjon commented 2 months ago

Hey @fosterseth: would you mind rebasing this and cutting out the 3.7 support?

bigfootjon commented 1 month ago

I've gone ahead and manually rebased this PR. @cacosandon would you mind rerunning your testing on the new version (also note @carltongibson's message in the other thread, you also need to apply https://github.com/django/channels_redis/pull/339)