channels icon indicating copy to clipboard operation
channels copied to clipboard

Is this a bug in get_capacity method ?

Open tinylambda opened this issue 5 years ago • 2 comments
trafficstars

https://github.com/django/channels/blob/42070bfe400048d7c8efe551d37f7ac0d26d8e7e/channels/layers.py#L123

I am reading the source code of Django channels, get_capacity(self, channel) method implements as this

def get_capacity(self, channel):
        """
        Gets the correct capacity for the given channel; either the default,
        or a matching result from channel_capacity. Returns the first matching
        result; if you want to control the order of matches, use an ordered dict
        as input.
        """
        for pattern, capacity in self.channel_capacity:
            if pattern.match(channel):
                return capacity
        return self.capacity

but I found that self.channel_capacity attribute is a dict:

    def __init__(self, expiry=60, capacity=100, channel_capacity=None):
        self.expiry = expiry
        self.capacity = capacity
        self.channel_capacity = channel_capacity or {}

If we iterate over self.channel_capacity directly we will get errors like this:

In [1]: d = {'x': 100, 'y': 200}                                                                                                                            

In [2]: for a, b in d: 
   ...:     print(a, b) 
   ...:                                                                                                                                                     
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-33301efab870> in <module>
----> 1 for a, b in d:
      2     print(a, b)
      3 

ValueError: not enough values to unpack (expected 2, got 1)

But it's OK to iterate over self.channel_capacity.items():

In [3]: for a, b in d.items(): 
   ...:     print(a, b) 
   ...:                                                                                                                                                     
x 100
y 200

Anyone can confirm this ? Thanks.

tinylambda avatar Mar 04 '20 16:03 tinylambda

It looks like the type of channel_capacity is swapped for a list of tuples with compile_capacities in channels_redis: https://github.com/django/channels_redis/blob/0c27648b60a1918c867b96a5ad3f67ef58ccceda/channels_redis/core.py#L200

I guess we should fix the BaseChannelLayer definition to always store a list of tuples? Or to always compile?

adamchainz avatar Mar 04 '20 19:03 adamchainz

It looks like the type of channel_capacity is swapped for a list of tuples with compile_capacities in channels_redis: https://github.com/django/channels_redis/blob/0c27648b60a1918c867b96a5ad3f67ef58ccceda/channels_redis/core.py#L200

I guess we should fix the BaseChannelLayer definition to always store a list of tuples? Or to always compile?

Yes. I think it's important to keep the code consistent, clear and correct.

tinylambda avatar Mar 05 '20 04:03 tinylambda