From 370db6488bfee5db9d90d675fd0ed8fae9baab0d Mon Sep 17 00:00:00 2001
From: Miguel Grinberg <miguelgrinberg50@gmail.com>
Date: Sat, 5 Mar 2016 23:10:37 -0800
Subject: [PATCH] Eliminate problematic _clean_rooms method

---
 socketio/base_manager.py   | 31 +++++++------------------------
 tests/test_base_manager.py |  7 +------
 2 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/socketio/base_manager.py b/socketio/base_manager.py
index 73be015..5f4743f 100644
--- a/socketio/base_manager.py
+++ b/socketio/base_manager.py
@@ -15,7 +15,6 @@ class BaseManager(object):
     def __init__(self):
         self.server = None
         self.rooms = {}
-        self.pending_removals = []
         self.callbacks = {}
 
     def initialize(self, server):
@@ -27,10 +26,8 @@ class BaseManager(object):
 
     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."""
@@ -56,7 +53,6 @@ class BaseManager(object):
 
     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]:
@@ -66,10 +62,11 @@ class BaseManager(object):
     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
 
@@ -126,17 +123,3 @@ class BaseManager(object):
         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 = []
diff --git a/tests/test_base_manager.py b/tests/test_base_manager.py
index 8ff9369..36c1086 100644
--- a/tests/test_base_manager.py
+++ b/tests/test_base_manager.py
@@ -30,7 +30,6 @@ class TestBaseManager(unittest.TestCase):
         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}})
@@ -47,7 +46,6 @@ class TestBaseManager(unittest.TestCase):
         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},
@@ -62,7 +60,6 @@ class TestBaseManager(unittest.TestCase):
         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},
@@ -75,7 +72,6 @@ class TestBaseManager(unittest.TestCase):
         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):
@@ -125,11 +121,10 @@ class TestBaseManager(unittest.TestCase):
         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')