Code review comment for lp:~vila/bzr/405745-http-hangs

I had to merge some refactoring done for bug #392127 so that the 2.4 band-aids stay smaller.

I changed the server.tearDown() into server.shutdown() as that better matches the SocketServer.py methods names.

I'm not particularly happy with having to override server_bind() twice, but that will be solved
with #392127 where the inheritance scheme is a bit different.

I may need to make connect_socket() a bit more reusable but I also plan to make more
test_server bits reusable so that could wait too.

Here is the differential diff (but it's ~200 lines while the full one is ~250):
=== modified file 'bzrlib/tests/http_server.py'
--- bzrlib/tests/http_server.py 2009-10-07 10:24:04 +0000
+++ bzrlib/tests/http_server.py 2009-10-08 08:25:53 +0000
@@ -317,27 +317,70 @@
         # the tests cases.
         self.test_case_server = test_case_server
         self._home_dir = test_case_server._home_dir
-
- def tearDown(self):
- """Called to clean-up the server.
-
- Since the server may be (surely is, even) in a blocking listen, we
- shutdown its socket before closing it.
- """
- # The server is listening for a last connection, let's give it:
- try:
- fake_conn = socket.create_connection(self.server_address)
- fake_conn.close()
- except socket.error, e:
- # But ignore connection errors as the point is to unblock the
- # server thread, it may happen that it's not blocked or even not
- # started (when something went wrong during test case setup
- # leading to self.setUp() *not* being called but self.tearDown()
- # still being called)
- pass
-
-
-class TestingHTTPServer(SocketServer.TCPServer, TestingHTTPServerMixin):
+ self.serving = False
+ self.is_shut_down = threading.Event()
+
+ def serve(self):
+ self.serving = True
+ self.is_shut_down.clear()
+ while self.serving:
+ try:
+ # Really a connection but the python framework is generic and
+ # call them requests
+ self.handle_request()
+ except socket.timeout:
+ pass
+ except (socket.error, select.error), e:
+ if e[0] == errno.EBADF:
+ # Starting with python-2.6, handle_request may raise socket
+ # or select exceptions when the server is shut down as we
+ # do.
+ pass
+ else:
+ raise
+ # Let's close the listening socket
+ self.server_close()
+ self.is_shut_down.set()
+
+ def connect_socket(self):
+ msg = "getaddrinfo returns an empty list"
+ for res in socket.getaddrinfo(*self.server_address):
+ af, socktype, proto, canonname, sa = res
+ sock = None
+ try:
+ sock = socket.socket(af, socktype, proto)
+ sock.connect(sa)
+ return sock
+
+ except socket.error, msg:
+ if sock is not None:
+ sock.close()
+ raise socket.error, msg
+
+ def shutdown(self):
+ """Stops the serve() loop.
+
+ Blocks until the loop has finished. This must be called while serve()
+ is running in another thread, or it will deadlock.
+ """
+ if not self.serving:
+ return
+ self.serving = False
+ # The server is listening for a last connection, let's give it:
+ try:
+ fake_conn = self.connect_socket()
+ fake_conn.close()
+ except socket.error, e:
+ # But ignore connection errors as the point is to unblock the
+ # server thread, it may happen that it's not blocked or even not
+ # started (when something went wrong during test case setup
+ # leading to self.setUp() *not* being called but self.tearDown()
+ # still being called)
+ pass
+ self.is_shut_down.wait()
+
+
+class TestingHTTPServer(TestingHTTPServerMixin, SocketServer.TCPServer):

     def __init__(self, server_address, request_handler_class,
                  test_case_server):
@@ -345,9 +388,15 @@
         SocketServer.TCPServer.__init__(self, server_address,
                                         request_handler_class)

-
-class TestingThreadingHTTPServer(SocketServer.ThreadingTCPServer,
- TestingHTTPServerMixin):
+ def server_bind(self):
+ SocketServer.TCPServer.server_bind(self)
+ if sys.version < (2, 5):
+ self.server_address = self.socket.getsockname()
+
+
+class TestingThreadingHTTPServer(TestingHTTPServerMixin,
+ SocketServer.ThreadingTCPServer,
+ ):
     """A threading HTTP test server for HTTP 1.1.

     Since tests can initiate several concurrent connections to the same http
@@ -382,6 +431,11 @@
             else:
                 raise

+ def server_bind(self):
+ SocketServer.ThreadingTCPServer.server_bind(self)
+ if sys.version < (2, 5):
+ self.server_address = self.socket.getsockname()
+

 class HttpServer(transport.Server):
     """A test server for http transports.
@@ -445,18 +499,18 @@
                 raise httplib.UnknownProtocol(proto_vers)
             else:
                 self._httpd = self.create_httpd(serv_cls, rhandler)
- host, self.port = self._httpd.socket.getsockname()
+ # Ensure we get the right port
+ host, self.port = self._httpd.server_address
         return self._httpd

     def _http_start(self):
         """Server thread main entry point. """
- self._http_running = False
+ server = None
         try:
             try:
- httpd = self._get_httpd()
+ server = self._get_httpd()
                 self._http_base_url = '%s://%s:%s/' % (self._url_protocol,
                                                        self.host, self.port)
- self._http_running = True
             except:
                 # Whatever goes wrong, we save the exception for the main
                 # thread. Note that since we are running in a thread, no signal
@@ -469,24 +523,8 @@

         # From now on, exceptions are taken care of by the
         # SocketServer.BaseServer or the request handler.
- while self._http_running:
- try:
- # Really an HTTP connection but the python framework is generic
- # and call them requests
- httpd.handle_request()
- except socket.timeout:
- pass
- except (socket.error, select.error), e:
- if e[0] == errno.EBADF:
- # Starting with python-2.6, handle_request may raise socket
- # or select exceptions when the server is shut down (as we
- # do).
- pass
- else:
- raise
- if self._httpd is not None:
- # Let the server properly close the listening socket
- self._httpd.server_close()
+ if server is not None:
+ server.serve()

     def _get_remote_url(self, path):
         path_parts = path.split(os.path.sep)
@@ -543,7 +581,7 @@
     def tearDown(self):
         """See bzrlib.transport.Server.tearDown."""
         self._http_running = False
- self._httpd.tearDown()
+ self._httpd.shutdown()
         self._http_thread.join()

     def get_url(self):

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py 2009-09-17 11:54:41 +0000
+++ bzrlib/tests/test_http.py 2009-10-08 07:25:52 +0000
@@ -325,10 +325,11 @@
         server = http_server.HttpServer()
         server.setUp()
         try:
- self.assertTrue(server._http_running)
+ self.assertTrue(server._httpd is not None)
+ self.assertTrue(server._httpd.serving)
         finally:
             server.tearDown()
- self.assertFalse(server._http_running)
+ self.assertFalse(server._httpd.serving)

     def test_create_http_server_one_zero(self):
         class RequestHandlerOneZero(http_server.TestingHTTPRequestHandler):