Rev 4734: Reduce the leaking http tests from ~200 to ~5. in file:///home/vila/src/bzr/bugs/392127-thread-leak/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/bugs/392127-thread-leak/

------------------------------------------------------------
revno: 4734
revision-id: v.ladeuil+lp at free.fr-20091007145554-j0njl74ji5o5v6cx
parent: v.ladeuil+lp at free.fr-20091007133103-bagf11imwtcenmzy
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 392127-thread-leak
timestamp: Wed 2009-10-07 16:55:54 +0200
message:
  Reduce the leaking http tests from ~200 to ~5.
  
  * bzrlib/tests/test_http.py:
  (WallRequestHandler._handle_one_request,
  PredefinedRequestHandler._handle_one_request): Be sure to use the
  proper wrapper to catch socket errors. This becomes more important
  when threaded servers are used and now properly shut down.
  
  * bzrlib/tests/http_server.py:
  (TestingHTTPRequestHandler.handle_one_request): Use a wrapper
  around the desired processing to share the socket errors handling.
  (TestingHTTPServerMixin.shutdown_client): Helper to
  (TestingThreadingHTTPServer): Collect clients and shut them down
  properly.
-------------- next part --------------
=== modified file 'bzrlib/tests/http_server.py'
--- a/bzrlib/tests/http_server.py	2009-10-07 13:31:03 +0000
+++ b/bzrlib/tests/http_server.py	2009-10-07 14:55:54 +0000
@@ -77,7 +77,7 @@
         connection early to avoid polluting the test results.
         """
         try:
-            SimpleHTTPServer.SimpleHTTPRequestHandler.handle_one_request(self)
+            self._handle_one_request()
         except socket.error, e:
             # Any socket error should close the connection, but some errors are
             # due to the client closing early and we don't want to pollute test
@@ -88,6 +88,9 @@
                                      errno.ECONNABORTED, errno.EBADF)):
                 raise
 
+    def _handle_one_request(self):
+        SimpleHTTPServer.SimpleHTTPRequestHandler.handle_one_request(self)
+
     _range_regexp = re.compile(r'^(?P<start>\d+)-(?P<end>\d+)$')
     _tail_regexp = re.compile(r'^-(?P<tail>\d+)$')
 
@@ -365,6 +368,28 @@
         self.is_shut_down.wait()
 
 
+    def shutdown_client(self, client_socket):
+        """Properly shutdown a client socket.
+
+        Under some circumstances (as in bug #383920), we need to force the
+        shutdown as python delays it until gc occur otherwise and the client
+        may hang.
+
+        This should be called only when no other thread is trying to use the
+        socket.
+        """
+        try:
+            # The request process has been completed, the thread is about to
+            # die, let's shutdown the socket if we can.
+            client_socket.shutdown(socket.SHUT_RDWR)
+        except (socket.error, select.error), e:
+            if e[0] in (errno.EBADF, errno.ENOTCONN):
+                # Right, the socket is already down
+                pass
+            else:
+                raise
+
+
 class TestingHTTPServer(TestingHTTPServerMixin, SocketServer.TCPServer):
 
     def __init__(self, server_address, request_handler_class,
@@ -393,23 +418,26 @@
         # process. This is prophylactic as we should not leave the threads
         # lying around.
         self.daemon_threads = True
+        # We collect the sockets used by the clients to we can close them when
+        # shutting down
+        self.clients = []
 
     def process_request_thread(self, request, client_address):
-        SocketServer.ThreadingTCPServer.process_request_thread(
-            self, request, client_address)
-        # Under some circumstances (as in bug #383920), we need to force the
-        # shutdown as python delays it until gc occur otherwise and the client
-        # may hang.
+        self.clients.append((request, threading.current_thread()))
         try:
-            # The request process has been completed, the thread is about to
-            # die, let's shutdown the socket if we can.
-            request.shutdown(socket.SHUT_RDWR)
-        except (socket.error, select.error), e:
-            if e[0] in (errno.EBADF, errno.ENOTCONN):
-                # Right, the socket is already down
-                pass
-            else:
-                raise
+            SocketServer.ThreadingTCPServer.process_request_thread(
+                self, request, client_address)
+        except Exception, e:
+            import pdb; pdb.set_trace()
+
+        self.shutdown_client(request)
+
+    def shutdown(self):
+        TestingHTTPServerMixin.shutdown(self)
+        # Let's close all our pending clients too
+        for c, t in self.clients:
+            self.shutdown_client(c)
+            t.join()
 
 
 class HttpServer(transport.Server):

=== modified file 'bzrlib/tests/test_http.py'
--- a/bzrlib/tests/test_http.py	2009-10-07 13:31:03 +0000
+++ b/bzrlib/tests/test_http.py	2009-10-07 14:55:54 +0000
@@ -610,7 +610,7 @@
 class WallRequestHandler(http_server.TestingHTTPRequestHandler):
     """Whatever request comes in, close the connection"""
 
-    def handle_one_request(self):
+    def _handle_one_request(self):
         """Handle a single HTTP request, by abruptly closing the connection"""
         self.close_connection = 1
 
@@ -1907,7 +1907,7 @@
     line.
     """
 
-    def handle_one_request(self):
+    def _handle_one_request(self):
         tcs = self.server.test_case_server
         requestline = self.rfile.readline()
         headers = self.MessageClass(self.rfile, 0)
@@ -2097,7 +2097,7 @@
 '''
         t = self.get_transport()
         # We must send a single line of body bytes, see
-        # PredefinedRequestHandler.handle_one_request
+        # PredefinedRequestHandler._handle_one_request
         code, f = t._post('abc def end-of-body\n')
         self.assertEqual('lalala whatever as long as itsssss\n', f.read())
         self.assertActivitiesMatch()