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

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5561
Proposed branch: lp:~vila/bzr/686008-spurious-https-failure
Merge into: lp:bzr
Diff against target: 66 lines (+18/-3)
3 files modified
bzrlib/tests/https_server.py (+12/-0)
bzrlib/tests/test_server.py (+3/-3)
doc/en/release-notes/bzr-2.3.txt (+3/-0)
To merge this branch: bzr merge lp:~vila/bzr/686008-spurious-https-failure
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Review via email: mp+42969@code.launchpad.net

Commit message

Catch spurious and bogus ssl TypeError exceptions.

Description of the change

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()

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :

Looks sane to me. Grammar nit:

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

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
17"""HTTPS test server, available when ssl python module is available"""17"""HTTPS test server, available when ssl python module is available"""
1818
19import ssl19import ssl
20import sys
2021
21from bzrlib.tests import (22from bzrlib.tests import (
22 http_server,23 http_server,
@@ -51,6 +52,17 @@
51 request.do_handshake()52 request.do_handshake()
52 return serving53 return serving
5354
55 def ignored_exceptions_during_shutdown(self, e):
56 if (sys.version < (2, 7) and isinstance(e, TypeError)
57 and e.args[0] == "'member_descriptor' object is not callable"):
58 # Fixed in python-2.7 (and some Ubuntu 2.6) there is a bug where
59 # the ssl socket fail to raise a socket.error when trying to read
60 # from a closed socket. This is rarely observed in practice but
61 # still make valid selftest runs fail if not caught.
62 return True
63 base = test_server.TestingTCPServerMixin
64 return base.ignored_exceptions_during_shutdown(self, e)
65
5466
55class TestingHTTPSServer(TestingHTTPSServerMixin,67class TestingHTTPSServer(TestingHTTPSServerMixin,
56 http_server.TestingHTTPServer):68 http_server.TestingHTTPServer):
5769
=== 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 @@
531 # Update the client description531 # Update the client description
532 self.clients.pop()532 self.clients.pop()
533 self.clients.append((request, client_address, t))533 self.clients.append((request, client_address, t))
534 # Propagate the exception handler since we must use the same one for534 # Propagate the exception handler since we must use the same one as
535 # connections running in their own threads than TestingTCPServer.535 # TestingTCPServer for connections running in their own threads.
536 t.set_ignored_exceptions(self.ignored_exceptions)536 t.set_ignored_exceptions(self.ignored_exceptions)
537 t.start()537 t.start()
538 started.wait()538 started.wait()
@@ -634,7 +634,7 @@
634 # server thread, it may happen that it's not blocked or even634 # server thread, it may happen that it's not blocked or even
635 # not started.635 # not started.
636 pass636 pass
637 # We start shutting down the client while the server itself is637 # We start shutting down the clients while the server itself is
638 # shutting down.638 # shutting down.
639 self.server.stop_client_connections()639 self.server.stop_client_connections()
640 # Now we wait for the thread running self.server.serve() to finish640 # Now we wait for the thread running self.server.serve() to finish
641641
=== 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 @@
61 suite. This can include new facilities for writing tests, fixes to 61 suite. This can include new facilities for writing tests, fixes to
62 spurious test failures and changes to the way things should be tested.62 spurious test failures and changes to the way things should be tested.
6363
64* Catch exceptions related to bug #637821 during test cleanup to avoid
65 spurious failures. (Vincent Ladeuil, #686008).
66
64* ``TestDebuntuExpansions`` was escaping the test isolation by calling the67* ``TestDebuntuExpansions`` was escaping the test isolation by calling the
65 wrong base class ``setUp``. (Vincent Ladeuil, #684622)68 wrong base class ``setUp``. (Vincent Ladeuil, #684622)
6669