Code review comment for lp://staging/~mbp/bzr/2.5-client-reconnect-819604

More precisely, on top of jelmer's branch, the following patch also resists the 1.000 stress test. (note that I wrongly tested it with TestingThreadingTCPServer instead of TestingTCPServer in DisconnectingServer at first, for even more stress tests).

   modified bzrlib/tests/test_bundle.py

=== modified file 'bzrlib/tests/test_bundle.py'
--- bzrlib/tests/test_bundle.py 2011-10-05 12:16:17 +0000
+++ bzrlib/tests/test_bundle.py 2011-11-28 10:57:48 +0000
@@ -17,6 +17,7 @@
 from cStringIO import StringIO
 import os
 import socket
+import SocketServer
 import sys
 import threading

@@ -41,13 +42,12 @@
 from bzrlib.bundle.serializer.v4 import BundleSerializerV4
 from bzrlib.repofmt import knitrepo
 from bzrlib.tests import (
+ features,
+ test_commit,
     test_read_bundle,
- test_commit,
+ test_server,
     )
 from bzrlib.transform import TreeTransform
-from bzrlib.tests import (
- features,
- )

 def get_text(vf, key):
@@ -1836,7 +1836,7 @@
         bundle, then the ConnectionReset error should be propagated.
         """
         # Instantiate a server that will provoke a ConnectionReset
- sock_server = _DisconnectingTCPServer()
+ sock_server = DisconnectingServer()
         self.start_server(sock_server)
         # We don't really care what the url is since the server will close the
         # connection without interpreting it
@@ -1844,35 +1844,21 @@
         self.assertRaises(errors.ConnectionReset, read_mergeable_from_url, url)

-class _DisconnectingTCPServer(object):
- """A TCP server that immediately closes any connection made to it."""
-
- def start_server(self):
- self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- self.sock.bind(('127.0.0.1', 0))
- self.sock.listen(1)
- self.port = self.sock.getsockname()[1]
- self.thread = threading.Thread(
- name='%s (port %d)' % (self.__class__.__name__, self.port),
- target=self.accept_and_close)
- self.thread.start()
-
- def accept_and_close(self):
- conn, addr = self.sock.accept()
- conn.shutdown(socket.SHUT_RDWR)
- conn.close()
+class DisconnectingHandler(SocketServer.BaseRequestHandler):
+ """A request handler that immediately closes any connection made to it."""
+
+ def handle(self):
+ self.request.close()
+
+
+class DisconnectingServer(test_server.TestingTCPServerInAThread):
+
+ def __init__(self):
+ super(DisconnectingServer, self).__init__(
+ ('127.0.0.1', 0),
+ test_server.TestingTCPServer,
+ DisconnectingHandler)

     def get_url(self):
- return 'bzr://127.0.0.1:%d/' % (self.port,)
-
- def stop_server(self):
- try:
- # make sure the thread dies by connecting to the listening socket,
- # just in case the test failed to do so.
- conn = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- conn.connect(self.sock.getsockname())
- conn.close()
- except socket.error:
- pass
- self.sock.close()
- self.thread.join()
+ """Return the url of the server"""
+ return "bzr://%s:%d/" % self.server.server_address

It relies on the existing test servers to avoid several pitfalls in the _DisconnectingTCPServer implementation:

- exceptions will be re-raised in the main thread should any of them occur in the server,
- no race condition between accept_and_close() and start_server()
- no need to shutdown(socket.SHUT_RDWR) from the server side, a regular close() is enough here (in DisconnectingHandler.handle())
- properly handle several requests (which I think was the root hang cause as pointed out by poolie)