[MERGE] Fix _get_line logic in smart protocol

[Martin Pool]

Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
Good catch.

=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py      2008-02-06 03:52:25 +0000
+++ bzrlib/smart/medium.py      2008-02-27 07:02:52 +0000
@@ -62,6 +62,14 @@
          # backing_transport could be passed to serve instead of 
__init__
          self.backing_transport = backing_transport
          self.finished = False
+        self.push_back = None
+
+    def _push_back(self, bytes):
+        assert self.push_back is None, (
+            "_push_back called when self.push is %r" % 
(self.push_back,))
+        if bytes == '':
+            return
+        self.push_back = bytes

Should be self.push, not self.push_back in the assertion message.

The variable should probably be underscored?  Asking that reminds me 
that
it's poor to have self.a and self._a with different meanings.

Please add a comment in the constructor about what it holds, and a
docstring on this method.


      def serve(self):
          """Serve requests until the client disconnects."""

@@ -163,11 +173,18 @@
                      return
                  protocol.accept_bytes(bytes)

-        self.push_back = protocol.excess_buffer
+        self._push_back(protocol.excess_buffer)

      def _get_bytes(self, desired_count):
          # We ignore the desired_count because on sockets it's more 
efficient to
          # read 4k at a time.
+        if self.push_back is not None:
+            assert self.push_back != '', (
+                'self.push_back should never be the empty string, which 
can be '
+                'confused with EOF')

In general I would recommend saying

    '%s.thing should blah blah' % (self, )

as it gives a bit more of a clue what went wrong, at least pointing you 
to
the right class and sometimes more than that.  For these objects we
probably will generally only have one so it's not a big deal.

+            bytes = self.push_back
+            self.push_back = None
+            return bytes
          return self.socket.recv(4096)

      def terminate_due_to_error(self):
@@ -217,6 +234,13 @@
              protocol.accept_bytes(bytes)

      def _get_bytes(self, desired_count):
+        if self.push_back is not None:
+            assert self.push_back != '', (
+                'self.push_back should never be the empty string, which 
can be '
+                'confused with EOF')
+            bytes = self.push_back
+            self.push_back = None
+            return bytes
          return self._in.read(desired_count)

There is a little duplicated code; maybe you should do

     if self._push_back_buffer is not None:
          return self._get_bytes_from_push_back(desired_count)

and maybe that function should return just the desired_count, since 
that's
cheap to do?  On the other hand some callers may be happy to get more,
like read_line.


      def terminate_due_to_error(self):

=== modified file 'bzrlib/tests/test_smart_transport.py'

+    def test_socket_stream_incomplete_request(self):
+        """The medium should still construct the right protocol version 
even if
+        the initial read only reads part of the request.
+
+        Specifically, it should correctly read the protocol version 
line even
+        if the partial read doesn't end in a newline.  An older, naive
+        implementation of _get_line in the server used to have a bug in 
that
+        case.
+        """
+        incomplete_request_bytes = protocol.REQUEST_VERSION_TWO + 'hel'
+        rest_of_request_bytes = 'lo\n'
+        expected_response = (
+            protocol.RESPONSE_VERSION_TWO + 'success\nok\x012\n')
+        server_sock, client_sock = self.portable_socket_pair()
+        server = medium.SmartServerSocketStreamMedium(
+            server_sock, None)
+        client_sock.sendall(incomplete_request_bytes)
+        server_protocol = server._build_protocol()
+        client_sock.sendall(rest_of_request_bytes)
+        server._serve_one_request(server_protocol)

Without tracing through the code, I'm not totally sure that the two 
writes
will be seen by the server as two reads.  But I presume you ran this
before putting in the fix and so you know it works.



For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C20080228150627.GG3192%40steerpike.home.puzzling.org%3E