group_send accepts messages that it can't decode

[lcannon]

Running on the latest versions of the channels packages:

/srv/venv/bin/pip freeze

asgiref==3.2.10
...
channels==2.4.0
channels-redis==3.0.1
...
daphne==2.5.0
...
Django==3.0.3

I started noticing that when I sent certain messages in my development environment (running in docker-compose with a redis:6.0.6 queue) my client would spontaneously disconnect. Unfortunately I couldn't see any errors, due to an open bug in django debug toolbar. After finally finding that out from another issue here, and rebuilding my environment without debug toolbar, I could finally see the error:

web_1      | MainProcess INFO WebSocket CONNECT /practice/ [172.18.0.5:52098]
web_1      | Exception inside application: NoneType is not allowed for map key
web_1      | Traceback (most recent call last):
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels/consumer.py", line 58, in __call__
web_1      |     await await_many_dispatch(
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels/utils.py", line 58, in await_many_dispatch
web_1      |     await task
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels/utils.py", line 50, in await_many_dispatch
web_1      |     result = task.result()
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels_redis/core.py", line 452, in receive
web_1      |     message_channel, message = await self.receive_single(
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels_redis/core.py", line 534, in receive_single
web_1      |     message = self.deserialize(content)
web_1      |   File "/srv/venv/lib/python3.8/site-packages/channels_redis/core.py", line 811, in deserialize
web_1      |     return msgpack.unpackb(message, raw=False)
web_1      |   File "msgpack/_unpacker.pyx", line 195, in msgpack._cmsgpack.unpackb
web_1      | ValueError: NoneType is not allowed for map key
web_1      | MainProcess INFO WebSocket DISCONNECT /practice/ [172.18.0.5:52098]

Which seems fair enough, and I have fixed our own code to not try to have None as a key. The reason I'm writing this bug report is, it seems like the code that is trying to send the message should raise an exception to have it localised there and easy to find & debug, but instead, this is in the code that receives that message from redis and is responsible for relaying it to the client.

In other words, the traceback above happened in my docker-compose ./manage.py runserver 0.0.0.0:8000, but what triggered it was a separate shell_plus window opened in the same application where I ran:

from asgiref.sync import async_to_sync
from channels.layers import get_channel_layer
prac = Practice.objects.first()
layer = get_channel_layer()
send = async_to_sync(layer.group_send)
send(prac.channel_name, {'type': 'send_message', 'text': {None: None}})

I would expect this to raise an exception about the None key, instead of happily popping it onto the queue to then raise an exception when being unpacked.

Of course, this seems ultimately to be a problem with msgpack:

In [10]: msgpack.packb({None: None})                                                     
Out[10]: b'\x81\xc0\xc0'

In [11]: msgpack.unpackb(b'\x81\xc0\xc0', raw=False)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-11-f271be043ea7> in <module>
----> 1 msgpack.unpackb(b'\x81\xc0\xc0', raw=False)

msgpack/_unpacker.pyx in msgpack._cmsgpack.unpackb()

ValueError: NoneType is not allowed for map key

so if you'd prefer I reported it on that project, I'm happy to.

[carltongibson]

I'm happy for you to report it there too, but I agree it might be nice if we could raise a helpful error.

Do you have any appetite to prototype that up so we can see what it would look like?

Thanks!

[lcannon]

The way I've handled it internally is to make a wrapper around group_send that ensures the message will unpack safely before sending it:

_layer = get_channel_layer()
_group_send = async_to_sync(_layer.group_send)


def group_send(group_name: str, message: dict) -> None:
    """Wrap native group_send with extra code to handle errors better. """
    # msgpack will pack messages it can't unpack -- we want to ensure if we've
    # been handed messages that can't be decoded the errors are thrown to the
    # *sending* code, not in the relaying code
    # This will avoid unwanted client disconnections from bad message sends
    msgpack.unpackb(msgpack.packb(message))

    _group_send(group_name, message)