Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrites RedisChannelLayer.receive specific channel section #165

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

augonis
Copy link

@augonis augonis commented Jul 26, 2019

I had a mystery bug where the channel_layer would stop receiving on specific channels after a bunch of receives got canceled.
Unable to reproduce the bug in clean conditions and unable to figure out what exactly was wrong, I just rewrote the suspect (RedisChannelLayer.receive).
My bug is gone and the tests still pass, so the pull request.

As a bonus, now simultaneously receiving on '!' channels with different prefixes should work (e.g. channel names produced by redisChannelLayer.new_channel(prefix='something')), witch was not the case before.

* also properly receives specific channels of different prefixes
@carltongibson
Copy link
Member

Hi @augonis. Interesting...

So, tests...

What’s our new buffer meant to offer that Queue doesn’t? Can we add some tests showing that in action?

@augonis
Copy link
Author

augonis commented Jul 29, 2019

The ReceiveBuffer is much more than a Queue, it manages waiters and buffers messages for all specific channels under the same 'real channel', as well as runs the receive loop for the 'real channel'.
It does have some Queue like bits in its implementation.
Only useful exposed are .loop and .get(channel), rest is implementation.

My goal was to simplify the control logic with the locks for the receiving as I suspect something there was behind the mystery bug.
Instead of locks I use a worker Task/loop that .single_receive(real_channel) that puts message in ReceiveBuffer that then either stores the massage for later or fulfills a getter Future.

I'll work on some tests, not familiar with pytest, but I'll learn.

* adds some tests for ReceiveBuffer
@augonis
Copy link
Author

augonis commented Jul 30, 2019

I added tests for the receive task handling, as basic receive is covered by existing tests.

P.S. I've put a limit on how many messages per specific channel receive Buffer will hold (hardcoded 20) unlike the original .receive that would buffer unlimited messages. Any thoughts?

@carltongibson
Copy link
Member

P.S. I've put a limit on how many messages per specific channel receive Buffer will hold (hardcoded 20) unlike the original .receive that would buffer unlimited messages. Any thoughts?

I wondering if we can tie this into capacity. (Then it's directly configurable...)

Overall this looks nice. It's my summer project to put together a set of decent releases for Daphne, Channels and here, so let's get this in. Thanks for the input! 🏅

@augonis
Copy link
Author

augonis commented Aug 1, 2019

Doable, but semantically and implementation wise it would muddy the meaning of capacity (and friends channel_capacity, get_capacity(channel)).

  • Currently capacity is configured/enforced on the sending side, having it affect receiving would be something new.
  • capacity also applies per real channel rather than per specific channel, so how it relates would have to be resolved

@carltongibson
Copy link
Member

Yes... but it should be exposed, and does it merit another config option? (Not sure...)

@augonis
Copy link
Author

augonis commented Aug 1, 2019

Should I remove the limit to unblock this PR and open a separate issue?

@augonis
Copy link
Author

augonis commented Aug 1, 2019

Just read django/channels#134, sounds similar my problem, this PR might address it.
django/channels#151 also may be related if that issue is only with specific channels.

@qeternity
Copy link
Contributor

What's the status with this?

@carltongibson
Copy link
Member

It just needs some time to review properly. I have some issues around Channels' AsyncHttpConsumer at the top of my list, and then channels_redis after that.

@davidfstr
Copy link

self.receive_buffers is added to but never removed from. Memory leak. Probably want to dereference receive buffers that become empty.

There's also the previously-existing memory leak related to channels that are written to but never written from, in #384 , which I don't advocate fixing in this commit because it's already big enough.

@augonis
Copy link
Author

augonis commented Nov 12, 2019

I'd expect receive_buffers to be long lived and few in number.
In fact for all existing applications there would be just one, as receiving on multiple real_channels previously was not supported, though implied by the ability to specify prefix in redisChannelLayer.new_channel(prefix='something').

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants