Add cancel_callback in dispatch

[fosterseth]

pairs with this PR django/channels_redis#339

to address a memory leak issue in pubsub backend django/channels_redis#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

[acu192]

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]

Future refactoring might be easier if you try/except an AttributeError here (if self.channel_layer.clean_channel doesn't exist, it will raise). This removes the hardcoded string which can make auto refactoring tools, etc fall over.

[fosterseth]

good call, you mean something like

cancel_callback = None
try:
  if callable(self.channel_layer.clean_channel):
    cancel_callback = functools.partial(self.channel_layer.clean_channel, self.channel_name)
except AttributeError:
  pass

?

[qeternity]

Yes, exactly what I had in mind

[fosterseth]

thanks, changed it

[qeternity]

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

[carltongibson]

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]

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

[bigfootjon]

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

[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 django/channels_redis#339)