Merge lp:~vila/bzr/383920-pycurl-hangs into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

The previous approach was not addressing the root cause.

The root cause is still a bit unclear as it involved python gc'ing a socket or not before
leaving pycurl polling it...

Anyway, this fix makes no hazardous assumption about the http protocol ;)

It explicitly shuts down the socket when the thread handling a http[s] connection is about
to finish. In our case, when the connection is closed by the server after sending a 404 error.

That may reduce the number of tests leaking threads, but it doesn't address all cases
(and it's hard to measure precisely anyway until we fix them all).

[John A Meinel (jameinel)]

You may want to wait for Robert to comment on it, but this seems to conform to everything he requested earlier.

review: Approve

[Robert Collins (lifeless)]

Thanks for digging deeper.

 review +1

review: Approve

=== modified file 'NEWS'
--- NEWS	2009-07-06 06:46:30 +0000
+++ NEWS	2009-07-08 12:35:11 +0000
@@ -79,6 +79,9 @@
   transforms.
   (Craig Hewetson, Martin Pool, #218206)
 
+* Force socket shutdown in threaded http test servers to avoid client hangs
+  (pycurl).  (Vincent Ladeuil, #383920).
+
 * The ``log+`` decorator, useful in debugging or profiling, could cause
   "AttributeError: 'list' object has no attribute 'next'".  This is now
   fixed.  The log decorator no longer shows the elapsed time or transfer

=== modified file 'bzrlib/tests/http_server.py'
--- bzrlib/tests/http_server.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/http_server.py	2009-07-08 12:35:12 +0000
@@ -14,6 +14,7 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+import BaseHTTPServer
 import errno
 import httplib
 import os
@@ -382,6 +383,23 @@
         # lying around.
         self.daemon_threads = True
 
+    def process_request_thread(self, request, client_address):
+        SocketServer.ThreadingTCPServer.process_request_thread(
+            self, request, client_address)
+        # Under some circumstances (as in bug #383920), we need to force the
+        # shutdown as python delays it until gc occur otherwise and the client
+        # may hang.
+        try:
+            # The request process has been completed, the thread is about to
+            # die, let's shutdown the socket if we can.
+            request.shutdown(socket.SHUT_RDWR)
+        except (socket.error, select.error), e:
+            if e[0] in (errno.EBADF, errno.ENOTCONN):
+                # Right, the socket is already down
+                pass
+            else:
+                raise
+
 
 class HttpServer(transport.Server):
     """A test server for http transports.

=== modified file 'bzrlib/tests/test_read_bundle.py'
--- bzrlib/tests/test_read_bundle.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_read_bundle.py	2009-07-08 12:35:12 +0000
@@ -84,70 +84,54 @@
 class TestReadBundleFromURL(TestTransportImplementation):
     """Test that read_bundle works properly across multiple transports"""
 
+    def setUp(self):
+        super(TestReadBundleFromURL, self).setUp()
+        self.bundle_name = 'test_bundle'
+        # read_mergeable_from_url will invoke get_transport which may *not*
+        # respect self._transport (i.e. returns a transport that is different
+        # from the one we want to test, so we must inject a correct transport
+        # into possible_transports first).
+        self.possible_transports = [self.get_transport(self.bundle_name)]
+        self._captureVar('BZR_NO_SMART_VFS', None)
+        wt = self.create_test_bundle()
+
+    def read_mergeable_from_url(self, url):
+        return bzrlib.bundle.read_mergeable_from_url(
+            url, possible_transports=self.possible_transports)
+
     def get_url(self, relpath=''):
         return bzrlib.urlutils.join(self._server.get_url(), relpath)
 
     def create_test_bundle(self):
         out, wt = create_bundle_file(self)
         if self.get_transport().is_readonly():
-            f = open('test_bundle', 'wb')
-            try:
-                f.write(out.getvalue())
-            finally:
-                f.close()
+            self.build_tree_contents([(self.bundle_name, out.getvalue())])
         else:
-            self.get_transport().put_file('test_bundle', out)
-            self.log('Put to: %s', self.get_url('test_bundle'))
+            self.get_transport().put_file(self.bundle_name, out)
+            self.log('Put to: %s', self.get_url(self.bundle_name))
         return wt
 
     def test_read_mergeable_from_url(self):
-        self._captureVar('BZR_NO_SMART_VFS', None)
-        wt = self.create_test_bundle()
-        if wt is None:
-            return
-        # read_mergeable_from_url will invoke get_transport which may *not*
-        # respect self._transport (i.e. returns a transport that is different
-        # from the one we want to test, so we must inject a correct transport
-        # into possible_transports first.
-        t = self.get_transport('test_bundle')
-        possible_transports = [t]
-        info = bzrlib.bundle.read_mergeable_from_url(
-                    unicode(self.get_url('test_bundle')),
-                    possible_transports=possible_transports)
+        info = self.read_mergeable_from_url(
+            unicode(self.get_url(self.bundle_name)))
         revision = info.real_revisions[-1]
         self.assertEqual('commit-1', revision.revision_id)
 
     def test_read_fail(self):
         # Trying to read from a directory, or non-bundle file
         # should fail with NotABundle
-        self._captureVar('BZR_NO_SMART_VFS', None)
-        wt = self.create_test_bundle()
-        if wt is None:
-            return
-
-        self.assertRaises(errors.NotABundle,
-            bzrlib.bundle.read_mergeable_from_url,
-            self.get_url('tree'))
-        self.assertRaises(errors.NotABundle,
-            bzrlib.bundle.read_mergeable_from_url,
-            self.get_url('tree/a'))
+        self.assertRaises(errors.NotABundle,
+                          self.read_mergeable_from_url, self.get_url('tree'))
+        self.assertRaises(errors.NotABundle,
+                          self.read_mergeable_from_url, self.get_url('tree/a'))
 
     def test_read_mergeable_respects_possible_transports(self):
-        t = self.get_transport('test_bundle')
-        if not isinstance(t, bzrlib.transport.ConnectedTransport):
+        if not isinstance(self.get_transport(self.bundle_name),
+                          bzrlib.transport.ConnectedTransport):
             # There is no point testing transport reuse for not connected
             # transports (the test will fail even).
-            return
-        self._captureVar('BZR_NO_SMART_VFS', None)
-        wt = self.create_test_bundle()
-        if wt is None:
-            return
-        # read_mergeable_from_url will invoke get_transport which may *not*
-        # respect self._transport (i.e. returns a transport that is different
-        # from the one we want to test, so we must inject a correct transport
-        # into possible_transports first.
-        possible_transports = [t]
-        url = unicode(self.get_url('test_bundle'))
-        info = bzrlib.bundle.read_mergeable_from_url(url,
-            possible_transports=possible_transports)
-        self.assertEqual(1, len(possible_transports))
+            raise tests.TestSkipped(
+                'Need a ConnectedTransport to test transport reuse')
+        url = unicode(self.get_url(self.bundle_name))
+        info = self.read_mergeable_from_url(url)
+        self.assertEqual(1, len(self.possible_transports))

=== modified file 'bzrlib/tests/test_transport_implementations.py'
--- bzrlib/tests/test_transport_implementations.py	2009-04-20 04:19:45 +0000
+++ bzrlib/tests/test_transport_implementations.py	2009-07-08 12:35:12 +0000
@@ -203,6 +203,13 @@
         for content, f in itertools.izip(contents, content_f):
             self.assertEqual(content, f.read())
 
+    def test_get_unknown_file(self):
+        t = self.get_transport()
+        files = ['a', 'b']
+        contents = ['contents of a\n',
+                    'contents of b\n',
+                    ]
+        self.build_tree(files, transport=t, line_endings='binary')
         self.assertRaises(NoSuchFile, t.get, 'c')
         self.assertListRaises(NoSuchFile, t.get_multi, ['a', 'b', 'c'])
         self.assertListRaises(NoSuchFile, t.get_multi, iter(['a', 'b', 'c']))
@@ -242,6 +249,9 @@
         for content, fname in zip(contents, files):
             self.assertEqual(content, t.get_bytes(fname))
 
+    def test_get_bytes_unknown_file(self):
+        t = self.get_transport()
+
         self.assertRaises(NoSuchFile, t.get_bytes, 'c')
 
     def test_get_with_open_write_stream_sees_all_content(self):