channels icon indicating copy to clipboard operation
channels copied to clipboard

Add cancel_callback in dispatch

Open fosterseth opened this issue 3 years ago • 6 comments

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:

  • if we hit an asyncio.CancelledError while awaiting in the receive() method, we get a chance to cleanup internal data related to the channel and all is well
  • if we hit an asyncio.CancelledError outside of the receive() method, we cancel the channels task, but we don't have a way to cleanup internal data related to that channel. That data will live forever until the process is restarted.

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

fosterseth avatar Nov 05 '22 01:11 fosterseth

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.

acu192 avatar Nov 11 '22 20:11 acu192

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

qeternity avatar Nov 11 '22 20:11 qeternity

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.

carltongibson avatar Nov 15 '22 13:11 carltongibson

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

fosterseth avatar Nov 16 '22 17:11 fosterseth

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

bigfootjon avatar Jul 10 '24 01:07 bigfootjon

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)

bigfootjon avatar Sep 03 '24 00:09 bigfootjon