Merge lp:~vila/bzr/test-server-races into lp:bzr

[Vincent Ladeuil]

Properly synchronize connection thread start with test server main thread.

This is a followup to the fix for bug #869366 that was sitting on disk for
no good reasons.

It removes one race in the test server when a connection thread is started.

Under rare circumstances (described in bug #869366) an exception in the
connection thread was raised in the server thread too early leading to
semi-hackish tests.

The connection thread will now wait for the server thread to resume its
normal course before processing the conection.

I see no reason why this should address the other issues we currently have
but I may be wrong.

On the other hand, this is clearly better and remove one source of possible
confusion.

[Martin Packman (gz)]

Okay, this all makes sense to me given the earlier merge proposal context.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/tests/test_server.py'
--- bzrlib/tests/test_server.py	2011-10-10 13:59:22 +0000
+++ bzrlib/tests/test_server.py	2011-12-01 12:49:45 +0000
@@ -432,12 +432,15 @@
     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
+        # The thread is not created yet, it will be updated in process_request
         self.clients.append((sock, addr, None))
         return sock, addr
 
-    def process_request_thread(self, started, stopped, request, client_address):
+    def process_request_thread(self, started, detached, stopped,
+                               request, client_address):
         started.set()
+        # We will be on our own once the server tells us we're detached
+        detached.wait()
         SocketServer.ThreadingTCPServer.process_request_thread(
             self, request, client_address)
         self.close_request(request)
@@ -446,12 +449,13 @@
     def process_request(self, request, client_address):
         """Start a new thread to process the request."""
         started = threading.Event()
+        detached = threading.Event()
         stopped = threading.Event()
         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, detached, stopped, request, client_address))
         # Update the client description
         self.clients.pop()
         self.clients.append((request, client_address, t))
@@ -460,12 +464,12 @@
         t.set_ignored_exceptions(self.ignored_exceptions)
         t.start()
         started.wait()
-        if debug_threads():
-            sys.stderr.write('Client thread %s started\n' % (t.name,))
         # If an exception occured during the thread start, it will get raised.
-        # In rare cases, an exception raised during the request processing may
-        # also get caught here (see http://pad.lv/869366)
         t.pending_exception()
+        if debug_threads():
+            sys.stderr.write('Client thread %s started\n' % (t.name,))
+        # Tell the thread, it's now on its own for exception handling.
+        detached.set()
 
     # The following methods are called by the main thread
 

=== modified file 'bzrlib/tests/test_test_server.py'
--- bzrlib/tests/test_test_server.py	2011-10-10 13:59:22 +0000
+++ bzrlib/tests/test_test_server.py	2011-12-01 12:49:45 +0000
@@ -212,10 +212,12 @@
 
         class FailingDuringResponseHandler(TCPConnectionHandler):
 
+            # We use 'request' instead of 'self' below because the test matters
+            # more and we need a container to properly set connection_thread.
             def handle_connection(request):
                 req = request.readline()
                 # Capture the thread and make it use 'caught' so we can wait on
-                # the even that will be set when the exception is caught. We
+                # the event that will be set when the exception is caught. We
                 # also capture the thread to know where to look.
                 self.connection_thread = threading.currentThread()
                 self.connection_thread.set_sync_event(caught)
@@ -232,20 +234,12 @@
         # Check that the connection thread did catch the exception,
         # http://pad.lv/869366 was wrongly checking the server thread which
         # works for TestingTCPServer where the connection is handled in the
-        # same thread than the server one but is racy for
-        # TestingThreadingTCPServer where the server thread may be in a
-        # blocking accept() call (or not).
-        try:
-            self.connection_thread.pending_exception()
-        except FailToRespond:
-            # Great, the test succeeded
-            pass
-        else:
-            # If the exception is not in the connection thread anymore, it's in
-            # the server's one. 
-            server.server.stopped.wait()
-            # The exception is available now
-            self.assertRaises(FailToRespond, server.pending_exception)
+        # same thread than the server one but was racy for
+        # TestingThreadingTCPServer. Since the connection thread detaches
+        # itself before handling the request, we are guaranteed that the
+        # exception won't leak into the server thread anymore.
+        self.assertRaises(FailToRespond,
+                          self.connection_thread.pending_exception)
 
     def test_exception_swallowed_while_serving(self):
         # We need to ensure the exception has been caught
@@ -260,9 +254,11 @@
 
         class FailingWhileServingConnectionHandler(TCPConnectionHandler):
 
+            # We use 'request' instead of 'self' below because the test matters
+            # more and we need a container to properly set connection_thread.
             def handle(request):
                 # Capture the thread and make it use 'caught' so we can wait on
-                # the even that will be set when the exception is caught. We
+                # the event that will be set when the exception is caught. We
                 # also capture the thread to know where to look.
                 self.connection_thread = threading.currentThread()
                 self.connection_thread.set_sync_event(caught)
@@ -270,6 +266,7 @@
 
         server = self.get_server(
             connection_handler_class=FailingWhileServingConnectionHandler)
+        self.assertEquals(True, server.server.serving)
         # Install the exception swallower
         server.set_ignored_exceptions(CantServe)
         client = self.get_client()
@@ -284,8 +281,8 @@
         # here). More precisely, the exception *has* been caught and captured
         # but it is cleared when joining the thread (or trying to acquire the
         # exception) and as such won't propagate to the server thread.
-        self.connection_thread.pending_exception()
-        server.pending_exception()
+        self.assertIs(None, self.connection_thread.pending_exception())
+        self.assertIs(None, server.pending_exception())
 
     def test_handle_request_closes_if_it_doesnt_process(self):
         server = self.get_server()