[2855] Use the same lock in the condition variable too

src/bin/memmgr/memmgr.py.in

@@ -139,12 +139,11 @@ class Memmgr(BIND10Server):
 
         # See the documentation for MemorySegmentBuilder on how the
         # following are used.
-        self._builder_cv = threading.Condition()
         self._builder_lock = threading.Lock()
+        self._builder_cv = threading.Condition(lock=self._builder_lock)
 
         self._builder = MemorySegmentBuilder(self._builder_sock,
                                              self._builder_cv,
-                                             self._builder_lock,
                                              self._builder_command_queue,
                                              self._builder_response_queue)
         self._builder_thread = threading.Thread(target=self._builder.run)
@@ -163,8 +162,7 @@ class Memmgr(BIND10Server):
         # This makes the MemorySegmentBuilder exit its main loop. It
         # should make the builder thread joinable.
         with self._builder_cv:
-            with self._builder_lock:
-                self._builder_command_queue.append('shutdown')
+            self._builder_command_queue.append('shutdown')
             self._builder_cv.notify_all()
 
         self._builder_thread.join()

src/lib/python/isc/memmgr/builder.py

@@ -19,7 +19,7 @@ class MemorySegmentBuilder:
     in the given order sequentially.
     """
 
-    def __init__(self, sock, cv, lock, command_queue, response_queue):
+    def __init__(self, sock, cv, command_queue, response_queue):
         """ The constructor takes the following arguments:
 
             sock: A socket using which this builder object notifies the
@@ -27,11 +27,10 @@ class MemorySegmentBuilder:
 
             cv: A condition variable object that is used by the main
                 thread to tell this builder object that new commands are
-                available to it.
-
-            lock: A lock object which should be acquired before using or
-                  modifying the contents of command_queue and
-                  response_queue.
+                available to it. Note that this is also used for
+                synchronizing access to the queues, so code that uses
+                MemorySegmentBuilder must use this condition variable's
+                lock object to synchronize its access to the queues.
 
             command_queue: A list of commands sent by the main thread to
                            this object. Commands should be executed
@@ -47,7 +46,6 @@ class MemorySegmentBuilder:
 
         self._sock = sock
         self._cv = cv
-        self._lock = lock
         self._command_queue = command_queue
         self._response_queue = response_queue
         self._shutdown = False
@@ -55,12 +53,12 @@ class MemorySegmentBuilder:
     def run(self):
         """ This is the method invoked when the builder thread is
             started.  In this thread, be careful when modifying
-            variables passed-by-reference in the constructor. If they are
-            reassigned, they will not refer to the main thread's objects
-            any longer. Any use of command_queue and response_queue must
-            be synchronized by acquiring the lock. This method must
-            normally terminate only when the 'shutdown' command is sent
-            to it.
+            variables passed-by-reference in the constructor. If they
+            are reassigned, they will not refer to the main thread's
+            objects any longer. Any use of command_queue and
+            response_queue must be synchronized by acquiring the lock in
+            the condition variable. This method must normally terminate
+            only when the 'shutdown' command is sent to it.
         """
 
         # Acquire the condition variable while running the loop.
@@ -70,9 +68,8 @@ class MemorySegmentBuilder:
                     self._cv.wait()
                 # Move the queue content to a local queue. Be careful of
                 # not making assignments to reference variables.
-                with self._lock:
-                    local_command_queue = self._command_queue.copy()
-                    self._command_queue.clear()
+                local_command_queue = self._command_queue.copy()
+                self._command_queue.clear()
 
                 # Run commands passed in the command queue sequentially
                 # in the given order.  For now, it only supports the
@@ -90,9 +87,7 @@ class MemorySegmentBuilder:
                         # main thread which would need to be
                         # notified. Instead return this in the response
                         # queue.
-                        with self._lock:
-                            self._response_queue.append(('bad_command',))
-
+                        self._response_queue.append(('bad_command',))
                         self._shutdown = True
                         break
 

src/lib/python/isc/memmgr/tests/builder_tests.py

@@ -30,11 +30,9 @@ class TestMemorySegmentBuilder(unittest.TestCase):
         self._builder_response_queue = []
 
         self._builder_cv = threading.Condition()
-        self._builder_lock = threading.Lock()
 
         self._builder = MemorySegmentBuilder(self._builder_sock,
                                              self._builder_cv,
-                                             self._builder_lock,
                                              self._builder_command_queue,
                                              self._builder_response_queue)
         self._builder_thread = threading.Thread(target=self._builder.run)
@@ -60,8 +58,7 @@ class TestMemorySegmentBuilder(unittest.TestCase):
         # Now that the builder thread is running, send it a bad
         # command. The thread should exit its main loop and be joinable.
         with self._builder_cv:
-            with self._builder_lock:
-                self._builder_command_queue.append('bad_command')
+            self._builder_command_queue.append('bad_command')
             self._builder_cv.notify_all()
 
         # Wait 5 seconds to receive a notification on the socket from
@@ -81,15 +78,15 @@ class TestMemorySegmentBuilder(unittest.TestCase):
         self.assertFalse(self._builder_thread.isAlive())
 
         # The command queue must be cleared, and the response queue must
-        # contain a response that a bad command was sent.
-        with self._builder_lock:
-            self.assertEqual(len(self._builder_command_queue), 0)
-            self.assertEqual(len(self._builder_response_queue), 1)
+        # contain a response that a bad command was sent. The thread is
+        # no longer running, so we can use the queues without a lock.
+        self.assertEqual(len(self._builder_command_queue), 0)
+        self.assertEqual(len(self._builder_response_queue), 1)
 
-            response = self._builder_response_queue[0]
-            self.assertTrue(isinstance(response, tuple))
-            self.assertTupleEqual(response, ('bad_command',))
-            self._builder_response_queue.clear()
+        response = self._builder_response_queue[0]
+        self.assertTrue(isinstance(response, tuple))
+        self.assertTupleEqual(response, ('bad_command',))
+        self._builder_response_queue.clear()
 
     def test_shutdown(self):
         """Tests that shutdown command exits the MemorySegmentBuilder
@@ -101,11 +98,10 @@ class TestMemorySegmentBuilder(unittest.TestCase):
         # Now that the builder thread is running, send it the shutdown
         # command. The thread should exit its main loop and be joinable.
         with self._builder_cv:
-            with self._builder_lock:
-                self._builder_command_queue.append('shutdown')
-                # Commands after 'shutdown' must be ignored.
-                self._builder_command_queue.append('bad_command_1')
-                self._builder_command_queue.append('bad_command_2')
+            self._builder_command_queue.append('shutdown')
+            # Commands after 'shutdown' must be ignored.
+            self._builder_command_queue.append('bad_command_1')
+            self._builder_command_queue.append('bad_command_2')
             self._builder_cv.notify_all()
 
         # Wait 5 seconds at most for the main loop of the builder to
@@ -114,10 +110,10 @@ class TestMemorySegmentBuilder(unittest.TestCase):
         self.assertFalse(self._builder_thread.isAlive())
 
         # The command queue must be cleared, and the response queue must
-        # be untouched (we don't use it in this test).
-        with self._builder_lock:
-            self.assertEqual(len(self._builder_command_queue), 0)
-            self.assertEqual(len(self._builder_response_queue), 0)
+        # be untouched (we don't use it in this test). The thread is no
+        # longer running, so we can use the queues without a lock.
+        self.assertEqual(len(self._builder_command_queue), 0)
+        self.assertEqual(len(self._builder_response_queue), 0)
 
 if __name__ == "__main__":
     isc.log.init("bind10-test")