Merge lp:~vila/bzr/686008-spurious-https-failure into lp:bzr

[Vincent Ladeuil]

Catch spurious and bogus ssl TypeError exceptions.

This fixes bug #686008 by making sure we're not affected by bug
#637821 for python versions that didn't get spiv's patch. In
other words: in some cases, during cleanup of tests involving an
https server, we get a TypeError exception while trying to read a
closed ssl socket instead of a socket.error.

Various tests has been affected by this bug making selftest
itself unreliable since the failure was rarely on the same test
nor even the same platform.

I was able to trigger the bug with a test but unfortunately not
in a reproducible way :-/ It needed no less than 3 breakpoints,
some prints and a bit of luck...

Anyhow, the fix itself is reduced enough in scope that it should
never mask real failures (this occurs only during a test cleanup
and has no effect on test validity).

So the fix works but since I can't trigger it on demand (and it
has served its purpose which was to check the exception), I've
removed the test from the proposal, here it is for the curious:

class TestBug686008(tests.TestCase):

    _test_needs_features = [tests.HTTPSServerFeature]

    def test_it(self):
        from bzrlib.tests import https_server
        import ssl
        trigger_bug = False
        class RequestHandler(http_server.TestingHTTPRequestHandler):

            def handle_one_request(self):
                if trigger_bug:
                    # Now, break the ssl socket
                    sock = socket.socket(socket.AF_INET)
                    self.connection = ssl.wrap_socket(sock)
                    self.rfile = self.connection.makefile('rb', self.rbufsize)
                http_server.TestingHTTPRequestHandler.handle_one_request(self)

            def do_HEAD(self):
                # Always succeds, leaving the connection open
                self.close_connection = 0
                self.send_response(200)
                self.end_headers()

        server = https_server.HTTPSServer(RequestHandler)
        self.start_server(server)
        self.assertIsInstance(server.server,
                              https_server.TestingThreadingHTTPSServer)
        t = _urllib.HttpTransport_urllib(server.get_url())
        self.addCleanup(t.disconnect)
        # We need a first clean request
        t.has('foo')
        # Now we force the server to choke
        trigger_bug = True
        server.stop_server()

[lp:~vila/bzr/686008-spurious-https-failure]

5564. By Vincent Ladeuil on 2010-12-07

Fix typo

[Martin Packman (gz)]

Looks sane to me. Grammar nit:

- ...same one than TestingTCPServer...
+ ...same one as TestingTCPServer...

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[lp:~vila/bzr/686008-spurious-https-failure]

5565. By Vincent Ladeuil on 2010-12-07

Fix spelling mistake.

=== modified file 'bzrlib/tests/https_server.py'
--- bzrlib/tests/https_server.py	2010-07-01 06:40:14 +0000
+++ bzrlib/tests/https_server.py	2010-12-07 17:04:17 +0000
@@ -17,6 +17,7 @@
 """HTTPS test server, available when ssl python module is available"""
 
 import ssl
+import sys
 
 from bzrlib.tests import (
     http_server,
@@ -51,6 +52,17 @@
             request.do_handshake()
         return serving
 
+    def ignored_exceptions_during_shutdown(self, e):
+        if (sys.version < (2, 7) and isinstance(e, TypeError)
+            and e.args[0] == "'member_descriptor' object is not callable"):
+            # Fixed in python-2.7 (and some Ubuntu 2.6) there is a bug where
+            # the ssl socket fail to raise a socket.error when trying to read
+            # from a closed socket. This is rarely observed in practice but
+            # still make valid selftest runs fail if not caught.
+            return True
+        base = test_server.TestingTCPServerMixin
+        return base.ignored_exceptions_during_shutdown(self, e)
+
 
 class TestingHTTPSServer(TestingHTTPSServerMixin,
                          http_server.TestingHTTPServer):

=== modified file 'bzrlib/tests/test_server.py'
--- bzrlib/tests/test_server.py	2010-08-31 08:24:17 +0000
+++ bzrlib/tests/test_server.py	2010-12-07 17:04:17 +0000
@@ -531,8 +531,8 @@
         # Update the client description
         self.clients.pop()
         self.clients.append((request, client_address, t))
-        # Propagate the exception handler since we must use the same one for
-        # connections running in their own threads than TestingTCPServer.
+        # Propagate the exception handler since we must use the same one as
+        # TestingTCPServer for connections running in their own threads.
         t.set_ignored_exceptions(self.ignored_exceptions)
         t.start()
         started.wait()
@@ -634,7 +634,7 @@
                 # server thread, it may happen that it's not blocked or even
                 # not started.
                 pass
-            # We start shutting down the client while the server itself is
+            # We start shutting down the clients while the server itself is
             # shutting down.
             self.server.stop_client_connections()
             # Now we wait for the thread running self.server.serve() to finish

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-03 11:16:25 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-07 17:04:17 +0000
@@ -61,6 +61,9 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* Catch exceptions related to bug #637821 during test cleanup to avoid
+  spurious failures. (Vincent Ladeuil, #686008).
+
 * ``TestDebuntuExpansions`` was escaping the test isolation by calling the
   wrong base class ``setUp``. (Vincent Ladeuil, #684622)