Rev 5256: The smart server leaks one thread (daemonic) for each connection. in file:///home/vila/src/bzr/experimental/leaking-tests/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/experimental/leaking-tests/

------------------------------------------------------------
revno: 5256
revision-id: v.ladeuil+lp at free.fr-20100531135112-3bn7mysnfl086f4g
parent: v.ladeuil+lp at free.fr-20100531125137-1r6ujrchth3zsi3s
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: catch-them-all
timestamp: Mon 2010-05-31 15:51:12 +0200
message:
  The smart server leaks one thread (daemonic) for each connection.
  
  * bzrlib/tests/test_smart_transport.py:
  (SmartTCPTests.start_server): Use stop_server for consistency.
  (SmartTCPTests.stop_server): Rename from tearDownServer for
  consistency.
  (TestServerSocketUsage.test_server_closes_listening_sock_on_shutdown_after_request):
  Keep track of the server URL explicitly.
  
  * bzrlib/smart/server.py (SmartTCPServer.serve_conn):
  Mark the origin of the leak.
-------------- next part --------------
=== modified file 'bzrlib/smart/server.py'
--- a/bzrlib/smart/server.py	2010-05-24 01:05:14 +0000
+++ b/bzrlib/smart/server.py	2010-05-31 13:51:12 +0000
@@ -173,6 +173,9 @@
         thread_name = 'smart-server-child' + thread_name_suffix
         connection_thread = threading.Thread(
             None, handler.serve, name=thread_name)
+        # FIXME: This thread is never joined, it should at least be collected
+        # somewhere so that tests that want to check for leaked threads can get
+        # rid of them -- vila 20100531
         connection_thread.setDaemon(True)
         connection_thread.start()
         return connection_thread

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- a/bzrlib/tests/test_smart_transport.py	2010-03-18 23:11:15 +0000
+++ b/bzrlib/tests/test_smart_transport.py	2010-05-31 13:51:12 +0000
@@ -1014,38 +1014,41 @@
         self.server = server.SmartTCPServer(self.backing_transport)
         self.server.start_background_thread('-' + self.id())
         self.transport = remote.RemoteTCPTransport(self.server.get_url())
-        self.addCleanup(self.tearDownServer)
+        self.addCleanup(self.stop_server)
         self.permit_url(self.server.get_url())
 
-    def tearDownServer(self):
+    def stop_server(self):
+        """Disconnect the client and stop the server.
+
+        This must be re-entrant as some tests will call it explicitly in
+        addition to the normal cleanup.
+        """
         if getattr(self, 'transport', None):
             self.transport.disconnect()
             del self.transport
         if getattr(self, 'server', None):
             self.server.stop_background_thread()
-            # XXX: why not .stop_server() -- mbp 20100106
             del self.server
 
 
 class TestServerSocketUsage(SmartTCPTests):
 
-    def test_server_setup_teardown(self):
-        """It should be safe to teardown the server with no requests."""
+    def test_server_start_stop(self):
+        """It should be safe to stop the server with no requests."""
         self.start_server()
-        server = self.server
         transport = remote.RemoteTCPTransport(self.server.get_url())
-        self.tearDownServer()
+        self.stop_server()
         self.assertRaises(errors.ConnectionError, transport.has, '.')
 
     def test_server_closes_listening_sock_on_shutdown_after_request(self):
         """The server should close its listening socket when it's stopped."""
         self.start_server()
-        server = self.server
+        server_url = self.server.get_url()
         self.transport.has('.')
-        self.tearDownServer()
+        self.stop_server()
         # if the listening socket has closed, we should get a BADFD error
         # when connecting, rather than a hang.
-        transport = remote.RemoteTCPTransport(server.get_url())
+        transport = remote.RemoteTCPTransport(server_url)
         self.assertRaises(errors.ConnectionError, transport.has, '.')
 
 
@@ -1187,7 +1190,7 @@
         self.transport.has('.')
         self.assertEqual([], self.hook_calls)
         # clean up the server
-        self.tearDownServer()
+        self.stop_server()
         # now it should have fired.
         self.assertEqual(result, self.hook_calls)
 
@@ -1206,7 +1209,7 @@
         self.transport.has('.')
         self.assertEqual([], self.hook_calls)
         # clean up the server
-        self.tearDownServer()
+        self.stop_server()
         # now it should have fired.
         self.assertEqual(result, self.hook_calls)