Repository: tornadoweb/tornado
Pull request #1356 (2015-02-28 19:17:26)

Fix memory leak in Semaphore.
Next I'll propose a reimplementation of Semaphore in terms of Condition in order to reuse its logic for cleaning up timed-out waiters.


Patch:
diff --git a/tornado/locks.py b/tornado/locks.py
index 8091d064..684945a8 100644
--- a/tornado/locks.py
+++ b/tornado/locks.py
@@ -34,7 +34,7 @@ class Condition(object):
         self._waiters = collections.deque()  # Futures.
         self._timeouts = 0
 
-    def __repr__(self):
+    def __str__(self):
         result = '<%s' % (self.__class__.__name__, )
         if self._waiters:
             result += ' waiters[%s]' % len(self._waiters)
@@ -89,7 +89,7 @@ class Event(object):
     def __init__(self):
         self._future = Future()
 
-    def __repr__(self):
+    def __str__(self):
         return '<%s %s>' % (
             self.__class__.__name__, 'set' if self.is_set() else 'clear')
 
@@ -181,7 +181,8 @@ class Semaphore(object):
     def release(self):
         """Increment the counter and wake one waiter."""
         self._value += 1
-        for waiter in self._waiters:
+        while self._waiters:
+            waiter = self._waiters.popleft()
             if not waiter.done():
                 self._value -= 1
 
@@ -238,7 +239,6 @@ class BoundedSemaphore(Semaphore):
         self._initial_value = value
 
     def release(self):
-        """Increment the counter and wake one waiter."""
         if self._value >= self._initial_value:
             raise ValueError("Semaphore released too many times")
         super(BoundedSemaphore, self).release()


Test patch:
diff --git a/tornado/test/locks_test.py b/tornado/test/locks_test.py
index c2930ca3..44dbcf73 100644
--- a/tornado/test/locks_test.py
+++ b/tornado/test/locks_test.py
@@ -33,12 +33,12 @@ class ConditionTest(AsyncTestCase):
                 self.history.append(key)
         future.add_done_callback(callback)
 
-    def test_repr(self):
+    def test_str(self):
         c = locks.Condition()
-        self.assertIn('Condition', repr(c))
-        self.assertNotIn('waiters', repr(c))
+        self.assertIn('Condition', str(c))
+        self.assertNotIn('waiters', str(c))
         c.wait()
-        self.assertIn('waiters', repr(c))
+        self.assertIn('waiters', str(c))
 
     @gen_test
     def test_notify(self):
@@ -170,7 +170,7 @@ class ConditionTest(AsyncTestCase):
 
 
 class EventTest(AsyncTestCase):
-    def test_repr(self):
+    def test_str(self):
         event = locks.Event()
         self.assertTrue('clear' in str(event))
         self.assertFalse('set' in str(event))
@@ -220,15 +220,15 @@ class SemaphoreTest(AsyncTestCase):
     def test_negative_value(self):
         self.assertRaises(ValueError, locks.Semaphore, value=-1)
 
-    def test_repr(self):
+    def test_str(self):
         sem = locks.Semaphore()
-        self.assertIn('Semaphore', repr(sem))
-        self.assertIn('unlocked,value:1', repr(sem))
+        self.assertIn('Semaphore', str(sem))
+        self.assertIn('unlocked,value:1', str(sem))
         sem.acquire()
-        self.assertIn('locked', repr(sem))
-        self.assertNotIn('waiters', repr(sem))
+        self.assertIn('locked', str(sem))
+        self.assertNotIn('waiters', str(sem))
         sem.acquire()
-        self.assertIn('waiters', repr(sem))
+        self.assertIn('waiters', str(sem))
 
     def test_acquire(self):
         sem = locks.Semaphore()
@@ -248,6 +248,7 @@ class SemaphoreTest(AsyncTestCase):
         sem.release()
         # Now acquire() is instant.
         self.assertTrue(sem.acquire().done())
+        self.assertEqual(0, len(sem._waiters))
 
     @gen_test
     def test_acquire_timeout(self):