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

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

Nasty bug here: pycurl was hanging when connecting in https mode to our test server
due to a missing Content-Length header in the python's BaseHttpServer implementation.

We didn't realize it sooner because it's triggered only when we test pycurl against
our https test server which requires both python2.6 and pycurl, which in turn seems
to be rather unsual.

A big thank you to Pavel, Russel and Martitza for bringing that up !

[Robert Collins (lifeless)]

I think your analysis of the problem is slightly off.

HTTP says (broadly):
- 1.1: connections are persistent unless otherwise noted/prevented
- 1.0: connections are non-persistent unless otherwise requested and not
       prevented
- all messages are delimited
 - by Content-Length or
 - TE or
 - dropping the connection

So, if:
 - we're sending 1.1 as the version
 - we were not sending Content-Length, nor doing TE
 - and not dropping the connection

then the server was fundamentally broken, not that the client was being
overly strict.

Is the diff accurate? it seems to have bundle stuff in it...

-Rob

[Vincent Ladeuil (vila)]

>>>>> "robert" == Robert Collins <email address hidden> writes:

    robert> I think your analysis of the problem is slightly off.
    robert> HTTP says (broadly):
    robert> - 1.1: connections are persistent unless otherwise noted/prevented
    robert> - 1.0: connections are non-persistent unless otherwise requested and not
    robert> prevented
    robert> - all messages are delimited
    robert> - by Content-Length or
    robert> - TE or
    robert> - dropping the connection

    robert> So, if:
    robert> - we're sending 1.1 as the version
    robert> - we were not sending Content-Length, nor doing TE
    robert> - and not dropping the connection

    robert> then the server was fundamentally broken, not that
    robert> the client was being overly strict.

Right. So the test server was:
- presenting itself as 1.1,
- not sending the Content-Length,
- sending 'Connection: close',
- closing the connection

and the client hanged, so following your arguments, it means the
*client* is broken, right ? Or is there still some grey area
because the server close on error while announcing 1.1 ?

Would you prefer KnownFailures for pycurl+https instead ?

    robert> Is the diff accurate? it seems to have bundle stuff in it...

Yes to both points, the commit messages says... d-a-m-n--i-t,
commit messages don't appear in mails, <suffering/> right, I
should have think about that and explain in the cover letter :-/

A test was added there (long ago, which breaks under the same conditions
than the others, to avoid further regressions there, I refactor
the three existing tests, since there was a bunch of duplication,
that makes some noise.

     Vincent

[Robert Collins (lifeless)]

On Tue, 2009-07-07 at 13:15 +0000, Vincent Ladeuil wrote:

> Right. So the test server was:
> - presenting itself as 1.1,
> - not sending the Content-Length,
> - sending 'Connection: close',
> - closing the connection

This is valid. How were we closing it? shutdown(both), or halfshutdown,
then close, or ...? Perhaps this question doesn't matter, if doing
exactly the same thing but adding the header was enough to stop the
symptoms.

> and the client hanged, so following your arguments, it means the
> *client* is broken, right ? Or is there still some grey area
> because the server close on error while announcing 1.1 ?

Is it using https, or just http? I can imagine that there is some https
teardown stuff that perhaps we aren't doing right.

> Would you prefer KnownFailures for pycurl+https instead ?

I'd prefer to know the real cause. If valid http behaviour makes pycurl
hang, then real servers on the internet will be able to make bzr hang,
and thats bad. As it stands, I feel that landing this is a bit of an
anti-fix, as it is a workaround we can't reasonably ask live internet
servers to make.

-Rob

[Martin Pool (mbp)]

31 + def send_error(self, code, message=None):
32 + """Send and log an error reply.
33 +
34 + Replace the bogus BaseHTTPServer.py with one that add the
35 + Content-Length header to address strict http clients.
36 + """

Please add a comment pointing to the bug so we know what particular problem this causes.

+ self.send_header("Content-Length", "%d" % len(content))

I'd be a bit concerned about this being wrong if the message happens to be unicode - this will get the length in characters and then it'll be written as bytes. But maybe that never happens.

57 + if self.command != 'HEAD' and code >= 200 and code not in (204, 304):
58 + self.wfile.write(content)

This should possibly handle OPTIONS too, though I doubt we ever send it and it may not even reach this function, so it's academic.

[Martin Pool (mbp)]

> On Tue, 2009-07-07 at 13:15 +0000, Vincent Ladeuil wrote:
>
> > Right. So the test server was:
> > - presenting itself as 1.1,
> > - not sending the Content-Length,
> > - sending 'Connection: close',
> > - closing the connection
>
> This is valid. How were we closing it? shutdown(both), or halfshutdown,
> then close, or ...? Perhaps this question doesn't matter, if doing
> exactly the same thing but adding the header was enough to stop the
> symptoms.

I can imagine some deadlocks occurring because they're both in the same process - perhaps the socket is relying on gc collect and it's not running before the deadlock occurs.

> > Would you prefer KnownFailures for pycurl+https instead ?
>
> I'd prefer to know the real cause. If valid http behaviour makes pycurl
> hang, then real servers on the internet will be able to make bzr hang,
> and thats bad. As it stands, I feel that landing this is a bit of an
> anti-fix, as it is a workaround we can't reasonably ask live internet
> servers to make.

I don't think we should hold this patch hostage to that. It's making the test server more correct and it's fixing a real problem that the test suite hangs. If we get reports that bzr hangs against real http/1.1 servers, which I don't remember seeing so far, then we can look at how to test that.

[Martin Pool (mbp)]

ie approve

review: Approve

[Robert Collins (lifeless)]

On Wed, 2009-07-08 at 00:09 +0000, Martin Pool wrote:

> I can imagine some deadlocks occurring because they're both in the
> same process - perhaps the socket is relying on gc collect and it's
> not running before the deadlock occurs.

Thats another possibility yes. Good catch.

> > > Would you prefer KnownFailures for pycurl+https instead ?
> >
> > I'd prefer to know the real cause. If valid http behaviour makes
> pycurl
> > hang, then real servers on the internet will be able to make bzr
> hang,
> > and thats bad. As it stands, I feel that landing this is a bit of an
> > anti-fix, as it is a workaround we can't reasonably ask live
> internet
> > servers to make.
>
> I don't think we should hold this patch hostage to that. It's making
> the test server more correct and it's fixing a real problem that the
> test suite hangs. If we get reports that bzr hangs against real
> http/1.1 servers, which I don't remember seeing so far, then we can
> look at how to test that.

The patch *does not* make the server more correct. It does prevent
hangs, but still, I think we should not stop looking at this till we
know the actual cause. Otherwise we've only half completed the root
cause analysis.

One reason we probably don't see reports of hangs, is that error cases
are relatively rare for bzr - we generally request resources that exist.

-Rob

[Vincent Ladeuil (vila)]

>>>>> "robert" == Robert Collins <email address hidden> writes:

    robert> On Tue, 2009-07-07 at 13:15 +0000, Vincent Ladeuil wrote:
    >> Right. So the test server was:
    >> - presenting itself as 1.1,
    >> - not sending the Content-Length,
    >> - sending 'Connection: close',
    >> - closing the connection

    robert> This is valid. How were we closing it? shutdown(both),

No

    robert> or halfshutdown,

Not sure what you mean.

    robert> then close, or ...?

Close the read and write parts of the socket. Simple closes.

    robert> Perhaps this question doesn't matter, if doing
    robert> exactly the same thing but adding the header was
    robert> enough to stop the symptoms.

I was about to either:
- look at the C code,
- test against real servers (via my local_test_server) plugins,
- keep thinking...

And then I thought about the send_error method that I didn't
fully trust in the past and thought, well, let's just try
that.. and it worked.

    >> and the client hanged, so following your arguments, it
    >> means the *client* is broken, right ? Or is there still
    >> some grey area because the server close on error while
    >> announcing 1.1 ?

    robert> Is it using https, or just http?

https *only*, http is fine...

    robert> I can imagine that there is some https teardown stuff
    robert> that perhaps we aren't doing right.

Whatever we do, a network connection loss will not, the client is
not supposed to hang either in that case no ?

    >> Would you prefer KnownFailures for pycurl+https instead ?

    robert> I'd prefer to know the real cause.

Cost/benefit analysis stopped me there, whatever the bug is, it
came from a change in either python or pycurl as I can guarantee
that at one point I had a python2.6/https/pycurl combo passing
the full test suite. It's a pity I didn't take note of the
precise versions involved.

    robert> If valid http behaviour makes pycurl hang, then real
    robert> servers on the internet will be able to make bzr
    robert> hang, and thats bad.

I totally share that view.

    robert> As it stands, I feel that landing this is a bit of an
    robert> anti-fix, as it is a workaround we can't reasonably
    robert> ask live internet servers to make.

Iff there are servers in the wild acting as described above (1.1,
close on error without Content-Length)...

My plan for that is to keep adding real servers to the
local_test_server plugin.

Overall, I think my time is better spent that way than fixing
bugs in pycurl instead of working on certificate validation for
urllib...

        Vincent

=== modified file 'NEWS'
--- NEWS	2009-07-06 06:46:30 +0000
+++ NEWS	2009-07-08 06:35:20 +0000
@@ -75,6 +75,10 @@
   of a remote repository.
   (Jelmer Vernooij, #332194)
 
+* Fix bogus ``send_error`` in python's BaseHTTPServer by inserting the missing
+  'Content-Length header. https pycurl clients can hang otherwise.
+  (Vincent Ladeuil, #383920).
+
 * Force deletion of readonly files during merge, update and other tree
   transforms.
   (Craig Hewetson, Martin Pool, #218206)

=== 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 06:35:20 +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
@@ -144,6 +145,35 @@
 
         return SimpleHTTPServer.SimpleHTTPRequestHandler.send_head(self)
 
+    def send_error(self, code, message=None):
+        """Send and log an error reply.
+
+        Replace the bogus BaseHTTPServer.py with one that add the
+        Content-Length header to address strict http clients.
+        """
+
+        try:
+            short, long = self.responses[code]
+        except KeyError:
+            short, long = '???', '???'
+        if message is None:
+            message = short
+        explain = long
+        self.log_error("code %d, message %s", code, message)
+        # using _quote_html to prevent Cross Site Scripting attacks (see bug
+        # #1100201)
+        content = (self.error_message_format %
+                   {'code': code,
+                    'message': BaseHTTPServer._quote_html(message),
+                    'explain': explain})
+        self.send_response(code, message)
+        self.send_header("Content-Length", "%d" % len(content))
+        self.send_header("Content-Type", self.error_content_type)
+        self.send_header('Connection', 'close')
+        self.end_headers()
+        if self.command != 'HEAD' and code >= 200 and code not in (204, 304):
+            self.wfile.write(content)
+
     def send_range_content(self, file, start, length):
         file.seek(start)
         self.wfile.write(file.read(length))

=== 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 06:35:20 +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 06:35:21 +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):