Memory leak if writing to a channel that is never read from later

[davidfstr]

It appears that if you write a message to a channel, for example via group_send, and no reader ever appears on that channel, the messages will remain in the in-memory queue channels.layers.channel_layers.backends['default'].receive_buffer indefinitely when using the RedisChannelLayer backend. In particular I have captured a server that has over 100k items in that dictionary.

One way to avoid this problem would be to extend the API for group_send with a time-to-live parameter so that messages would expire over time if they weren't read. Thoughts?

My pip freeze, in case it's useful:

channels==2.1.2
channels-redis==2.2.1

[carltongibson]

Thoughts?

Right, yes. 🙂

• First off I guess, we should make clearing out stale items easier.
• Then allowing a default TTL.
• Then allowing that per-message.

[davidfstr]

I have discovered that RedisChannelLayer already has a concept of a "group expiry", which we may want to hook into for the "set a default TTL" scenario:

class RedisChannelLayer(BaseChannelLayer):
    def __init__(..., group_expiry=86400, ...):
        self.group_expiry = group_expiry  # channels_redis.core:46

This is alluded to briefly in Channels documentation:

Groups are a broadcast system that:

• ...
• Provides group expiry for clean-up of connections whose disconnect handler didn’t get to run (e.g. power failure)

I was unable to find documentation for changing the group_expiry of RedisChannelLayer, but reading the source for channel/layers.py:58@_make_backend suggests that I could put in settings.py something like:

    CHANNEL_LAYERS = {
        'default': {
            'BACKEND': 'channels_redis.core.RedisChannelLayer',
            'CONFIG': {
                'hosts': ...,
                'prefix': ...,
                'group_expiry': 86400,  # seconds?; 24 hours
            },
        },
    }

Do you think it would be appropriate to hook into the existing "group_expiry" concept, or do you think a separate "message_expiry" (or similar) would be more appropriate?

[carltongibson]

No, let's use what's there.

I was unable to find documentation ...

Ultimately I think this is the issue: I end up having to look at the channels_redis source to work out what the options are. So first step would be a PR explaining what's already possible.

(Maybe that'd be enough)

[davidfstr]

Agreed that the documentation should be extended to show how to configure the built-in backend types.

Unfortunately group_expiry doesn't actually control expiration of messages in the receive_buffer. So there still needs to be some logic added to perform expiration.

Fix 1 Sketch

• First off I guess, we should make clearing out stale items easier.

I propose that receive_buffer keep track of the last time a message was read from it, on a per-channel basis. And that an API (expire_unread_channels) is provided on RedisChannelLayer that can clear any channels that were not read within the last N seconds. Example usage:

channel_layer = channels.layers.get_channel_layer()
assert isinstance(channel_layer, RedisChannelLayer)
channel_layer.expire_unread_channels(ttl=300)  # seconds; = 5 minutes; defaults to 5 minutes

Fix 2 Sketch

• Then allowing a default TTL.

Extend Fix 1 above so that RedisChannelLayer spawns a thread that calls expire_unread_channels() on itself every M seconds, for some M (perhaps group_expiry/2?). This thread would use the configured group_expiry (mentioned earlier) as the default TTL to pass to expire_unread_channels().

[davidfstr]

Prior art: Sessions has a similar problem with clearing expired session records. It provides the following API:

clear_expired()

• Removes expired sessions from the session store. This class method is called by clearsessions.

We could spell expire_unread_channels() instead as clear_expired() for consistency, although I like the original name a bit better. :)

[carltongibson]

Yes, good. I like the build up.

If you're keen here, can I ask you to look at #165 first? It looks good at first sight, but I haven't had the bandwidth to sit down with it properly, so another pair of eyes there would be super handy!

[davidfstr]

@carltongibson Is #165 potentially related to this bug?

Reviewing that would require me to learn in detail how the current and proposed receiving system works, which take a block of time (2hrs) that is difficult for me to schedule in a timely fashion.

[carltongibson]

Hey @davidfstr.

...which take a block of time...

Yep. Open source. :) (This is the exact hold-up there... BANDWIDTH). No stress, no rush — if you don't have capacity to look at it, no problem! But... if you can slowly have it on the back burner, that would be a great help. (As I say, no stress about it!!!)

Is it related? To the extent that it's on the short list of improvements to be made here, and we're looking at touching the receive buffer, yes.

But... have a glance at it and see what you think. I'm happy to accept your input as it's available, so you tell me. 🙂

[davidfstr]

Got it. Touches the receive buffer. So we'd have to reconcile these changes eventually anyway.

I'll keep the review in my queue. Projections at work have me looking at this again in 3 weeks, but I may get lucky and find some time earlier.

[carltongibson]

OK, thanks @davidfstr! (As I say, no stress! It's OSS 🙂)

[jheld]

Hello all! What's the status here? And what's the (average?) severity of it not being fixed (if that's even a reasonable question)?

All of the interactions into channels redis on my project happen on a celery worker so worst case the worker fails (as opposed to ballooning RAM on an API machine), but again, rather find a way around that!

[davidfstr]

Status: Bug doesn't hurt enough that anyone has put in the effort to fix it yet.

To workaround this (and other kinds of slowish memory leaks), I've configured most of my web services to restart after serving (N + random jitter) requests. Gunicorn in particular has a config option for this.