Skip to content

Commit

Permalink
Eliminate problematic _clean_rooms method
Browse files Browse the repository at this point in the history
  • Loading branch information
@miguelgrinberg
miguelgrinberg committed Mar 6, 2016
1 parent 74e9ab1 commit 370db64
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 30 deletions.
31 changes: 7 additions & 24 deletions socketio/base_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class BaseManager(object):
def __init__(self):
self.server = None
self.rooms = {}
self.pending_removals = []
self.callbacks = {}

def initialize(self, server):
Expand All @@ -27,10 +26,8 @@ def get_namespaces(self):

def get_participants(self, namespace, room):
"""Return an iterable with the active participants in a room."""
for sid, active in six.iteritems(self.rooms[namespace][room]):
if active:
yield sid
self._clean_rooms()
for sid, active in six.iteritems(self.rooms[namespace][room].copy()):
yield sid

def connect(self, sid, namespace):
"""Register a client connection to a namespace."""
Expand All @@ -56,7 +53,6 @@ def disconnect(self, sid, namespace):

def enter_room(self, sid, namespace, room):
"""Add a client to a room."""
self._clean_rooms() # ensure our rooms are up to date first
if namespace not in self.rooms:
self.rooms[namespace] = {}
if room not in self.rooms[namespace]:
Expand All @@ -66,10 +62,11 @@ def enter_room(self, sid, namespace, room):
def leave_room(self, sid, namespace, room):
"""Remove a client from a room."""
try:
# do not delete immediately, just mark the client as inactive
# _clean_rooms() will do the clean up when it is safe to do so
self.rooms[namespace][room][sid] = False
self.pending_removals.append((namespace, room, sid))
del self.rooms[namespace][room][sid]
if len(self.rooms[namespace][room]) == 0:
del self.rooms[namespace][room]
if len(self.rooms[namespace]) == 0:
del self.rooms[namespace]
except KeyError:
pass

Expand Down Expand Up @@ -126,17 +123,3 @@ def _generate_ack_id(self, sid, namespace, callback):
id = six.next(self.callbacks[sid][namespace][0])
self.callbacks[sid][namespace][id] = callback
return id

def _clean_rooms(self):
"""Remove all the inactive room participants."""
for namespace, room, sid in self.pending_removals:
try:
del self.rooms[namespace][room][sid]
except KeyError:
# failures here could mean there were duplicates so we ignore
continue
if len(self.rooms[namespace][room]) == 0:
del self.rooms[namespace][room]
if len(self.rooms[namespace]) == 0:
del self.rooms[namespace]
self.pending_removals = []
7 changes: 1 addition & 6 deletions tests/test_base_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ def test_disconnect(self):
self.bm.enter_room('123', '/foo', 'bar')
self.bm.enter_room('456', '/foo', 'baz')
self.bm.disconnect('123', '/foo')
self.bm._clean_rooms()
self.assertEqual(self.bm.rooms['/foo'], {None: {'456': True},
'456': {'456': True},
'baz': {'456': True}})
Expand All @@ -47,7 +46,6 @@ def test_disconnect_default_namespace(self):
self.assertTrue(self.bm.is_connected('123', '/foo'))
self.bm.disconnect('123', '/foo')
self.assertFalse(self.bm.is_connected('123', '/foo'))
self.bm._clean_rooms()
self.assertEqual(self.bm.rooms['/'], {None: {'456': True},
'456': {'456': True}})
self.assertEqual(self.bm.rooms['/foo'], {None: {'456': True},
Expand All @@ -62,7 +60,6 @@ def test_disconnect_twice(self):
self.bm.disconnect('123', '/foo')
self.bm.disconnect('123', '/')
self.bm.disconnect('123', '/foo')
self.bm._clean_rooms()
self.assertEqual(self.bm.rooms['/'], {None: {'456': True},
'456': {'456': True}})
self.assertEqual(self.bm.rooms['/foo'], {None: {'456': True},
Expand All @@ -75,7 +72,6 @@ def test_disconnect_all(self):
self.bm.enter_room('456', '/foo', 'baz')
self.bm.disconnect('123', '/foo')
self.bm.disconnect('456', '/foo')
self.bm._clean_rooms()
self.assertEqual(self.bm.rooms, {})

def test_disconnect_with_callbacks(self):
Expand Down Expand Up @@ -125,11 +121,10 @@ def test_get_participants(self):
self.bm.connect('456', '/')
self.bm.connect('789', '/')
self.bm.disconnect('789', '/')
self.assertEqual(self.bm.rooms['/'][None]['789'], False)
self.assertNotIn('789', self.bm.rooms['/'][None])
participants = list(self.bm.get_participants('/', None))
self.assertEqual(len(participants), 2)
self.assertNotIn('789', participants)
self.assertNotIn('789', self.bm.rooms['/'][None])

def test_leave_invalid_room(self):
self.bm.connect('123', '/foo')
Expand Down

9 comments on commit 370db64

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the "clean_rooms" functionality been deprecated indefinitely?

@miguelgrinberg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand your question. What specific functionality you refer to? The _clean_rooms() function was internal and did not provide any useful features to applications.

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's subtle, but the useful feature in this case is the clean up of the sessions that are saved when clients attempt to connect. I noticed that references to sids stick around in cases where a connect attempt is made, but is unsuccessful at some point. In my case specifically, it happens when the client is blocked for not having valid credentials (with a 401 error).

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth mentioning that I am using flask_login.login_required for validating credentials.

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so unless you're not supposed to use flask_login.login_required with socketio.on('connect'), doing so seems to cause a resource leak. The idea is, if abort(401) is called by login_manager.py when connecting, an sid is left lingering for the attempted connection.

I can put together an example of this if you like. Package versions I'm using:
Flask (0.10.1)
Flask-Login (0.4.0)
Flask-SocketIO (2.9.0)
python-engineio (1.7.0)
python-socketio (1.7.6)

@miguelgrinberg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Flask-Login's login_required decorator is not compatible with Flask-SocketIO, it only works for HTTP routes. You can use current_user in your Flask-SocketIO events, but login_required is not going to work.

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, is that due to a limitation at a lower level, or could this be considered as a possible enhancement for Flask-SocketIO?

@miguelgrinberg
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never intended for Flask-Login authentication to work. That was made for HTTP and this is WebSocket, so it's not compatible. I can't really say that it is impossible to make it work because I didn't really invest the time to see if anything can be done, but I think it is unlikely. If you want to give it a shot and find a way to use login_required on the connect handler, I'll add it as a feature. But consider that all the redirect stuff that Flask-Login does when the user is not logged in will have to be disabled, for this usage you want the authentication error to return False from the connect handler, or maybe a 401. Definitely not a redirect to the login page.

@robolivable
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds good. I'll look into this when I can as I am interested to see if it could work.

Please sign in to comment.