Rev 6176: Close the request when we don't process it. in http://bazaar.launchpad.net/~jameinel/bzr/2.5-no-hanging-teardown

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/2.5-no-hanging-teardown

------------------------------------------------------------
revno: 6176
revision-id: john at arbash-meinel.com-20111003065619-5ov2sw2643rtjqdf
parent: pqm at pqm.ubuntu.com-20110928153255-zb9flucmyyducc0m
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: 2.5-no-hanging-teardown
timestamp: Mon 2011-10-03 08:56:19 +0200
message:
  Close the request when we don't process it.
  Also close the request when a failure occurs during processing.
  
  The code used to only close the request at server shutdown,
  but that meant that bugs in the client-server RPC (such as
  the server raising an exception, etc) could cause a client
  to hang, which blocks the test suite from shutting down
  the server.
-------------- next part --------------
=== modified file 'bzrlib/tests/test_server.py'
--- a/bzrlib/tests/test_server.py	2011-09-15 13:58:23 +0000
+++ b/bzrlib/tests/test_server.py	2011-10-03 06:56:19 +0000
@@ -19,6 +19,7 @@
 import SocketServer
 import sys
 import threading
+import traceback
 
 
 from bzrlib import (
@@ -312,7 +313,8 @@
                 self.process_request(request, client_address)
             except:
                 self.handle_error(request, client_address)
-                self.close_request(request)
+        else:
+            self.close_request(request)
 
     def get_request(self):
         return self.socket.accept()
@@ -332,8 +334,15 @@
         # The following can be used for debugging purposes, it will display the
         # exception and the traceback just when it occurs instead of waiting
         # for the thread to be joined.
-
         # SocketServer.BaseServer.handle_error(self, request, client_address)
+        # We call close_request manually, because we are going to raise an
+        # exception. The SocketServer implementation calls:
+        #   handle_error(...)
+        #   close_request(...)
+        # But because we raise the exception, close_request will never be
+        # triggered. This helps client not block waiting for a response when
+        # the server gets an exception.
+        self.close_request(request)
         raise
 
     def ignored_exceptions_during_shutdown(self, e):
@@ -419,7 +428,7 @@
         SocketServer.ThreadingTCPServer.__init__(self, server_address,
                                                  request_handler_class)
 
-    def get_request (self):
+    def get_request(self):
         """Get the request and client address from the socket."""
         sock, addr = TestingTCPServerMixin.get_request(self)
         # The thread is not create yet, it will be updated in process_request
@@ -440,8 +449,8 @@
         t = TestThread(
             sync_event=stopped,
             name='%s -> %s' % (client_address, self.server_address),
-            target = self.process_request_thread,
-            args = (started, stopped, request, client_address))
+            target=self.process_request_thread,
+            args=(started, stopped, request, client_address))
         # Update the client description
         self.clients.pop()
         self.clients.append((request, client_address, t))

=== modified file 'bzrlib/tests/test_test_server.py'
--- a/bzrlib/tests/test_test_server.py	2011-09-15 12:03:23 +0000
+++ b/bzrlib/tests/test_test_server.py	2011-10-03 06:56:19 +0000
@@ -19,6 +19,7 @@
 import SocketServer
 import threading
 
+
 from bzrlib import (
     osutils,
     tests,
@@ -94,7 +95,7 @@
             self.server_class = server_class
         if connection_handler_class is None:
             connection_handler_class = TCPConnectionHandler
-        server =  test_server.TestingTCPServerInAThread(
+        server = test_server.TestingTCPServerInAThread(
             ('localhost', 0), self.server_class, connection_handler_class)
         server.start_server()
         self.addCleanup(server.stop_server)
@@ -148,7 +149,7 @@
         class CantConnect(Exception):
             pass
 
-        class FailingConnectionHandler(TCPConnectionHandler):
+        class FailingConnectionHandler(SocketServer.BaseRequestHandler):
 
             def handle(self):
                 raise CantConnect()
@@ -158,15 +159,14 @@
         # The server won't fail until a client connect
         client = self.get_client()
         client.connect((server.host, server.port))
+        # We make sure the server wants to handle a request, but the request is
+        # guaranteed to fail. However, the server should make sure that the
+        # connection gets closed, and stop_server should then raise the
+        # original exception.
+        client.write('ping\n')
         try:
-            # Now we must force the server to answer by sending the request and
-            # waiting for some answer. But since we don't control when the
-            # server thread will be given cycles, we don't control either
-            # whether our reads or writes may hang.
-            client.sock.settimeout(0.1)
-            client.write('ping\n')
-            client.read()
-        except socket.error:
+            self.assertEqual('', client.read())
+        except socket.error, e:
             pass
         # Now the server has raised the exception in its own thread
         self.assertRaises(CantConnect, server.stop_server)
@@ -219,6 +219,13 @@
         # been swallowed.
         server.pending_exception()
 
+    def test_handle_request_closes_if_it_doesnt_process(self):
+        server = self.get_server()
+        client = self.get_client()
+        server.server.serving = False
+        client.connect((server.host, server.port))
+        self.assertEqual('', client.read())
+
 
 class TestTestingSmartServer(tests.TestCase):