miguelgrinberg / Flask-SocketIO

Socket.IO integration for Flask applications.
MIT License
5.36k stars 890 forks source link

Room parameter of send() was silently renamed between v4 and v5. #1771

Closed stackp closed 2 years ago

stackp commented 2 years ago

Greetings and thanks for your work !

After upgrading from 4 to 5, the following code started ignoring the room parameter and sending a message to everyone. Looking at the changelog, it appears that the room parameter has been renamed to.

I believe it would help avoid surprises if an error was thrown when room is passed. Or to treat room as an alias for to (like python-socketio) and issuing a deprecation warning.

from flask_socketio import SocketIO

socketio = SocketIO(path="/ws")

# [...]

socketio.send(
    values,
    json=True,
    room="room_name",
    skip_sid=skip_sid,
)

Here's the requirements.txt diff :

-Flask-SocketIO==4.2.1
+Flask-SocketIO==5.1.1
-python-engineio==3.12.1
+python-engineio==4.3.0
-python-socketio==4.5.1
+python-socketio==5.5.0

Thanks again, Pierre

miguelgrinberg commented 2 years ago

You are correct that the room parameter was renamed to to a while ago, but I did not remove room, and did not intend for it to be ignored.

The to parameter is obtained as follows:

to = kwargs.pop('to', kwargs.pop('room', None))

Which seems correct. There also unit tests that use room instead of to, such as https://github.com/miguelgrinberg/Flask-SocketIO/blob/main/test_socketio.py#L501.

So I don't understand why this isn't working for you.

stackp commented 2 years ago

Thank you for your reply @miguelgrinberg.

Looking at the code, it looks like this is indeed handled correctly in socketio.send() but there is a subtle bug in socketio.SocketIO().send()

  1. self.send(..., room="name") inserts to=None in the kwargs ofself.emit() => kwargs={"to":None, "room": "name"})
  2. in self.emit() kwargs.pop('to', kwargs.pop('room', None)) returns None even though room == "name" because to does exists in kwargs

So, the bug in a nutshell:

# self.emit(..., to=None, **{"room": "name"})
>>> kwargs = {"to": None, "room": "name"}
>>> to = kwargs.pop('to', kwargs.pop('room', None))
>>> print(to)
None

Cheers! :)

miguelgrinberg commented 2 years ago

Okay, yeah, you did mention it was on send() and not on emit(). Sorry, I missed that. I'll correct this.