Rev 6053: Fix a naughty race in test_server_crash_while_responding in file:///home/vila/src/bzr/bugs/869366-test-server-race/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/bugs/869366-test-server-race/

------------------------------------------------------------
revno: 6053
revision-id: v.ladeuil+lp at free.fr-20111007134112-seo2r1u8y0pf97a7
parent: pqm at pqm.ubuntu.com-20111006162704-p7n73ermqotrq8y7
fixes bug: https://launchpad.net/bugs/869366
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 869366-test-server-race
timestamp: Fri 2011-10-07 15:41:12 +0200
message:
  Fix a naughty race in test_server_crash_while_responding
-------------- next part --------------
=== modified file 'bzrlib/tests/test_server.py'
--- a/bzrlib/tests/test_server.py	2011-02-10 12:37:27 +0000
+++ b/bzrlib/tests/test_server.py	2011-10-07 13:41:12 +0000
@@ -287,7 +287,6 @@
 
     def serve(self):
         self.serving = True
-        self.stopped.clear()
         # We are listening and ready to accept connections
         self.started.set()
         try:
@@ -508,7 +507,7 @@
             sync_event=self.server.started,
             target=self.run_server)
         self._server_thread.start()
-        # Wait for the server thread to start (i.e release the lock)
+        # Wait for the server thread to start (i.e. release the lock)
         self.server.started.wait()
         # Get the real address, especially the port
         self.host, self.port = self.server.server_address

=== modified file 'bzrlib/tests/test_test_server.py'
--- a/bzrlib/tests/test_test_server.py	2011-02-08 16:26:23 +0000
+++ b/bzrlib/tests/test_test_server.py	2011-10-07 13:41:12 +0000
@@ -86,9 +86,6 @@
         for name in
         ('TestingTCPServer', 'TestingThreadingTCPServer')]
 
-    # Set by load_tests()
-    server_class = None
-
     def get_server(self, server_class=None, connection_handler_class=None):
         if server_class is not None:
             self.server_class = server_class
@@ -174,6 +171,9 @@
     def test_server_crash_while_responding(self):
         sync = threading.Event()
         sync.clear()
+        # The thread that will serve the client
+        self.connection_thread = None
+        test = self # So the handler can set connection_thread
         class FailToRespond(Exception):
             pass
 
@@ -181,7 +181,10 @@
 
             def handle_connection(self):
                 req = self.rfile.readline()
-                threading.currentThread().set_sync_event(sync)
+                # Capture the thread and make it use 'sync' so we can wait on
+                # the even set when the exception is caught
+                test.connection_thread = threading.currentThread()
+                test.connection_thread.set_sync_event(sync)
                 raise FailToRespond()
 
         server = self.get_server(
@@ -190,20 +193,31 @@
         client.connect((server.host, server.port))
         client.write('ping\n')
         sync.wait()
-        self.assertRaises(FailToRespond, server.pending_exception)
+        # 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.
+        self.assertRaises(FailToRespond,
+                          self.connection_thread.pending_exception)
 
     def test_exception_swallowed_while_serving(self):
         sync = threading.Event()
         sync.clear()
+        # The thread that will serve the client
+        self.connection_thread = None
+        test = self # So the handler can set connection_thread
         class CantServe(Exception):
             pass
 
         class FailingWhileServingConnectionHandler(TCPConnectionHandler):
 
-            def handle(self):
-                # We want to sync with the thread that is serving the
-                # connection.
-                threading.currentThread().set_sync_event(sync)
+            def handle(request):
+                # Capture the thread and make it use 'sync' so we can wait on
+                # the even set when the exception is caught
+                test.connection_thread = threading.currentThread()
+                test.connection_thread.set_sync_event(sync)
                 raise CantServe()
 
         server = self.get_server(
@@ -216,5 +230,7 @@
         # Wait for the exception to propagate.
         sync.wait()
         # The connection wasn't served properly but the exception should have
-        # been swallowed.
-        server.pending_exception()
+        # been swallowed (see test_server_crash_while_responding remark about
+        # http://pad.lv/869366 explaining why we can't check the server thread
+        # here).
+        self.connection_thread.pending_exception()

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- a/doc/en/release-notes/bzr-2.4.txt	2011-10-06 16:27:04 +0000
+++ b/doc/en/release-notes/bzr-2.4.txt	2011-10-07 13:41:12 +0000
@@ -75,6 +75,9 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* Fix the race for TestingThreadingTCPServer in
+  test_server_crash_while_responding. (Vincent Ladeuil, #869366)
+
 * Really corrupt the pack file without depending on a special length or value.
   (Vincent Ladeuil, #807032)