[MERGE] Fix _get_line logic in smart protocol

Andrew Bennetts andrew at canonical.com
Thu Feb 28 15:06:27 GMT 2008


Hi,

This bundle does a few things:

 * fixes a bug in the way _get_line is implemented in bzrlib.smart.medium.  It
   erroneously assumed that _get_bytes(1) cannot return more than 1 byte.  It's
   basically due to lucky co-incidences that this hasn't caused us significant
   trouble (beyond me wasting time debugging weird test failures while
   implementing v3 of the protocol).
 * adds a “_push_back” method to SmartServerStreamMedium to manage the buffer of
   already-read-but-not-yet-consumed bytes (a.k.a. the “push back buffer”...
   better name ideas are welcome).  The idea here is to make it harder to make
   mistakes with the logic, compared to the previous idiom of just accessing the
   instance variable directly.
 * makes _get_bytes actually use the push_back buffer!  Again, it's due to lucky
   coincidences that this hasn't caused bugs.

I've added tests for the new _get_bytes implementations.  However, I'm not sure
that test_pipe_stream_incomplete_request will work correctly on Windows.  The
pipe functions involved should be available there according to the standard
library docs, but I'm not 100% certain the behaviour is the same.  If someone
with Windows could test this change, that would be good.

-Andrew.

-------------- next part --------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: andrew.bennetts at canonical.com-20080227070252-\
#   pivlmm0zkmt0m2wh
# target_branch: http://bazaar-vcs.org/bzr/bzr.dev
# testament_sha1: aac404e190acf8a2cc43582c4b5f267faccd8356
# timestamp: 2008-02-29 01:43:00 +1100
# source_branch: http://people.ubuntu.com/~andrew/bzr/robust-push-back
# base_revision_id: pqm at pqm.ubuntu.com-20080225092119-bk1won32t9nw4h6u
# 
# Begin patch
=== 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
 
     def serve(self):
         """Serve requests until the client disconnects."""
@@ -127,16 +135,19 @@
 
         :returns: a string of bytes ending in a newline (byte 0x0A).
         """
-        # XXX: this duplicates SmartClientRequestProtocolOne._recv_tuple
-        line = ''
-        while not line or line[-1] != '\n':
-            new_char = self._get_bytes(1)
-            line += new_char
-            if new_char == '':
+        newline_pos = -1
+        bytes = ''
+        while newline_pos == -1:
+            new_bytes = self._get_bytes(1)
+            bytes += new_bytes
+            if new_bytes == '':
                 # Ran out of bytes before receiving a complete line.
-                break
+                return bytes
+            newline_pos = bytes.find('\n')
+        line = bytes[:newline_pos+1]
+        self._push_back(bytes[newline_pos+1:])
         return line
-
+ 
 
 class SmartServerSocketStreamMedium(SmartServerStreamMedium):
 
@@ -147,7 +158,6 @@
             into blocking mode.
         """
         SmartServerStreamMedium.__init__(self, backing_transport)
-        self.push_back = ''
         sock.setblocking(True)
         self.socket = sock
 
@@ -155,7 +165,7 @@
         while protocol.next_read_size():
             if self.push_back:
                 protocol.accept_bytes(self.push_back)
-                self.push_back = ''
+                self.push_back = None
             else:
                 bytes = self._get_bytes(4096)
                 if bytes == '':
@@ -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')
+            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)
 
     def terminate_due_to_error(self):

=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py	2008-01-22 06:56:59 +0000
+++ bzrlib/tests/test_smart_transport.py	2008-02-27 07:02:52 +0000
@@ -434,6 +434,7 @@
         # now disconnect again: this should not do anything, if disconnection
         # really did disconnect.
         medium.disconnect()
+
     
     def test_tcp_client_ignores_disconnect_when_not_connected(self):
         # Doing a disconnect on a new (and thus unconnected) TCP medium
@@ -733,6 +734,68 @@
         server._serve_one_request(SampleRequest('x'))
         self.assertTrue(server.finished)
         
+    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)
+        server_sock.close()
+        self.assertEqual(expected_response, client_sock.recv(50),
+                         "Not a version 2 response to 'hello' request.")
+        self.assertEqual('', client_sock.recv(1))
+
+    def test_pipe_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')
+        # Make a pair of pipes, to and from the server
+        to_server, to_server_w = os.pipe()
+        from_server_r, from_server = os.pipe()
+        to_server = os.fdopen(to_server, 'r', 0)
+        to_server_w = os.fdopen(to_server_w, 'w', 0)
+        from_server_r = os.fdopen(from_server_r, 'r', 0)
+        from_server = os.fdopen(from_server, 'w', 0)
+        server = medium.SmartServerPipeStreamMedium(
+            to_server, from_server, None)
+        # Like test_socket_stream_incomplete_request, write an incomplete
+        # request (that does not end in '\n') and build a protocol from it.
+        to_server_w.write(incomplete_request_bytes)
+        server_protocol = server._build_protocol()
+        # Send the rest of the request, and finish serving it.
+        to_server_w.write(rest_of_request_bytes)
+        server._serve_one_request(server_protocol)
+        to_server_w.close()
+        from_server.close()
+        self.assertEqual(expected_response, from_server_r.read(),
+                         "Not a version 2 response to 'hello' request.")
+        self.assertEqual('', from_server_r.read(1))
+        from_server_r.close()
+        to_server.close()
+
     def test_pipe_like_stream_with_two_requests(self):
         # If two requests are read in one go, then two calls to
         # _serve_one_request should still process both of them as if they had

# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWfjrVM8ACET/gGRUAgB77///
f////r////BgD93yilAKCgoFAAJFFBQAAAAAKAAAHNMRkZNMmgGQ0ZDJkAAAMjTI0DCGQc0xGRk0
yaAZDRkMmQAAAyNMjQMIZBzTEZGTTJoBkNGQyZAAADI0yNAwhkCUIjQTKn4kwyk9Eek9MgmmmmQN
Awh6gaNGTQBzTEZGTTJoBkNGQyZAAADI0yNAwhkBVEQE0BGjQaATJoExGhpop4ozJhEw0JjKQQsB
FQATrxWGnCOTIoq25MxDKs0tSudPGLEwzP7fjHVn/jFKeRmUy7vJmnVwNbCL7lWD/K11piMX6MHS
sceN6v4M1DVTFUfyT/mY1MHevzLTcolYopPuOHQGNSWXQbmLCePjyCQEUR4pvmwz7rW7eBYsEEe/
NCO2XrmHDKDYZ8+il11rOvX1lW1W21Zvsujg7Oyte4IjBKEIiQiNznc3Y6eI6XKOv4nkVdUVSUS3
Nfqjv+syXmhH8zoJTITMTIqzBRm6QTVya3UUzFRQyRKSM1crCSwaJmeorRazLEp650LqY2ra0uqs
VwtYXXKpuo6KRbmsdc426vJtOVLfESo54f7TUsl/hKOxa7WL5PqTyQ52C4iOYI4HOI38yeBBrDre
NtzitRgXPeHwMJV1JLZELLTGZU3NrPZFzDcxapaFIlakrVj3v1X24mZiq4JSzKywZ2Y14u/DRjGO
UtTXg1NBpWrWte2RmZNMxRgwRSy/EvYMbMVWDYZFeKY87tUyM0k0YRoSzFiyKtthtbWy7XRpYrNr
OM6qxcmJzMbWV5e4csFKV4XtbW0vXpT0MV7yeXiX/8+t3vItbi+szNnhODhebx83Dbbblre8l63V
ERff6HWsfSvfBWIRi+mXKjrS63Q8iryeCh70lx9ybnZ600RcuzpglK5KMksm130SJVHbJ/WWMwEC
Ugz3K9z9Arh44WI3aNHHRvE1cbLzzM4RYv5li2dhbqqsp4LHhnpVoXX+linWUPbRpYZnJOtnRTbX
bkrbZamnmVZ53sKXvqIPvR9CP1a0t6j0GLOSk6QeCx6CTqaVjzIYPTcSG5hY4kpiXPfa9S/wy6zS
o5lExRJzoiKTyUWnrJiEc4uRDSwSSXLyhgwCW/Y27+LGyMzFtvOIM2DDOIYVQcoyoHAuqqBcksmq
5zEZJBK9Vh3+K45FqhZalERkvQtudBpqTl7Fcy5gsL30W2Fy8vVRbVqVXZ8kM9wargULy6qUUjNC
ViWpV9G9DqcPOhlsZfe0+RbKUNSWqYvTGqqi4rVceYtxepTWfwTCdrkzqMzQl6pK0rQvqlWFELeK
WqnEEo8lpBVSlZBGaBSNEgzCTLAkBiGZOBMfLJhoMKu/UU/26eJrXfY1RFUd9WtrdPUlRqdPWeU0
ODFhscvDcpmQtqRC4yLlN2xRXA5VGNiIjg83OzrGiMoUQ5uEqGdzqRDPnv58YsiGmWlzcN7NczaV
MGdgaXA8gdp+jHOWBoO1DXSWqnHw1v02qQ1bNvL4FjSgtvDhcK+61pXZ23GoZ7L5zL6Xrl65R1sz
kamDWqoKC8VzyF6lVfTEz8ZKyvI2cJPUNQSmJz3EEmJGKtzOrpWFkYavq2LIZmKOLGmaNTgXLfOf
3+v+vS+/qzcG5ys7TtLaBwR47lhnjBmXOO45F6GPFduc97m2NlDZ7c7Vm0c91jBlSrF1H9s+ne1y
dTy4rGk/nn16+Pir2KYzPH8Zai4nBZEZgko47JmawXpcsRKy3JVqXK8dmbhXMGTcpsW2xHt2tVux
Ri8/x+w13S1TsrhDVqi5DMhOphdyL2zBeF3Armalyq2MqMCbO9Ruu2oZZmZi4WlJYvmycuKu/Q16
c4MdVSyL1y4zLroaU55UJ97w/xpXMlFF7TXf8XYHBgcK6tb1eDSW/sS2NyVvBm4Tld/ktxcqjkX3
Xc9+R6MqL1mOqjPnUa17gab1pPu4jT9HwhkuViWt1uZsslMExyGyI/02oWGxG2IXajsfA9yXdJH5
pK91j+TxKIl9NIR5kxEbn7JNugl9Dj5EpSralKkRKWlgjtf0R7XrVXqGKVFVERHzn8nyOrw/M+75
ffza2dXY0xKqCoKop0BxEkCvb6uf8xnaPPGZi5dWo+go+26wmOIL5YSH6BTsPSd3WModJ38RuF/k
NJNcHYJykQnZY6tTc9jwVRoJO7q/1m1vT2/T+GqxMdpwmg2pcqUlsQ8zP+Bc42B+WKPn0MyNrqa/
IycebY/f0I95cXZe7LO8Vg8C9FO8xpM3v6LPYn5L+k/Uzt6gt0HqtMoSBsnCLnHhb7682+HQZeQV
XEpwnuL4Xm5yIRf0NjSSSiYo6EzPvhvTaUd0TEKQSlKHU+ZLoXxgqjBdD+CUpXmWfBlxTMpiYiUf
VomUMokxlEyXlUZ0PvVRHn+ppsJ7R/Ac2jSmmn4IibqoiVgZytkvYQvohAxnom8gbQ2oiKc6hlEX
ouKs+zJQ/u7XhFeqFFYTFalF6Ii5G/8pS2EOcStfufpHxu4z4sJkv1tix83OaN55nc7e7mdzBe/F
5HgXWo9iEfze1oWKux09XG2aF/IZ55URGVjI25NN5LuOk2LCxLodCqKJqmZRKPtbSI425oPHfzN3
jc3OdvctxGZxESoUU/YiJqDWkMZOWFbteo49coNjYGuSDREN8wWJhRKYSmGqNbshUJQKsJmI+RQ0
sRx6T770slItlVVuoorYi2FFXOq7nTwtzic2ZisdBlEYIXKHM4w+H262Rc0uDfratjPMd/+oZLe9
GdEa+SOE1AufsG+p1foo0PjUVfRb6uha1kqLJVSeuYLmLFfLZKZwVRQmL8Ey9Pq6nnd3cnjc/Os4
7ni8izbjyVWZr72jRdyrnWlwPcS5UfkXR0zyEuVuLYhvYIXJTCy3lHFMGCaok4eZclpYGh8qUpRz
8EYpb705sdCUQSydTWVaVkrI+y58bFERpaDxhRkisBKHBJqTwyinHtViIZSj8VINRMkwhpJIWwR3
SLJaCSrOjhjSdVZRakihSIVdzliJjig2AuiOIooQiYQ9PqfGzEZmlpdEUUSmiiUqRkQdjweDzOQ7
+jre1ez4KlrI9Nl70uZtMXYjwTwI3+qj+J6zoMU8RvyrBvnfNFJUczzl0aIlHT6oiPbEajiDf/xc
dB9J2uV4RNSiij9OgsfrwUQi6UExfHC5/QqfYmEeO75jSiy5mGXJ53EjqhC/Z2VcFhxc6Ii17Fav
e3ojRC843S+hJ7UczM4XPtjUakjJ8UmZngpOmKu1c54hriIbfU1LcV+l5V6x4bBpSQ9b8EvKuRwz
y6iG2prjqWrHIiMHjmQ/NoOLsC/qsbA3wg9ZK/7sv+ntaXqvhXriG5qQ6SxknAkzKKJVURRERPB1
NaH/ejtRK5KI2YsHNMGSIPd08ELe5LhhO8jZ+OhQzNRR1IaXnY54eb1rTNvdirtcehMSkEofedyH
wQ/Op+/gOBMYRDHofAlSFbEcJ9b9yVW5V6W0+W/Y4UMzF7kXqwZnkQ6Hnq1LlyYwVKlitamdWxaf
tEbFYXCyR8JGofMwWiS4pCJYqoiLoslIkkklCY5wXnuS7Pco71ij9G1jil8ylEoiKKRRoBsaERGE
WIoiVkQWyiOw8FR4nq70X86ZUiHGcjkHWmLAr8cah5zlaPOFi5RAmIYUiFiI6SQ/CIexi1XNWSIj
J8SLGln0aSRWIbBa9q68WolDBGaGZpTnyDFF3keujVJ5XldKFT2l6OKG59xoIr/0yw7ENWMQ4iLt
t6HT956dccDgJXNEJJd7y6GdqTELkomEoj5CvoD4uWsDHpDQCG0qTqH+te4cbw9z4mRVBkVEZFDq
l9swh0RD3u9FOhyI4krIiepVmZILKEzJKEpT7HK5j/ivQ+suOlKKvQDk6Dhz3Nb1RwuiYbYj+q31
QjDpEc17ISmeNQlRFCiKSpCkl6YRCiOwKDPvWqotJQlVSU7FLJqLCZJS0FFhKURH8aHlisnoLoSy
Xn174bXNETES596ip2LXi6njpQ5F+CkUTCVFEr1EY1URZncMtKUIwMUda9F4o7GCSSUoiw+dtQ1K
uJMQwiiNqJiNCFhRDQmCMZLmeIWxRe4Fm4MWqeMmbWqM82y5kmLQ9i0WJKpUR50PP7Okoo0Fo4Hz
4FEmRSFCZ+ISrCUVWxSFV1mUyuKRGc43hEQdzj5EvFRRDUZlHTLoSiOTYtSbFh8NjxDWsRjm66RW
tIyCrGIbUdKPYVhWW1pqUREUohqcQKGpSIseVL23mxzGWLwbTvNLtyZRbEHpP3zG1G84URUx1wUO
Ja4ERFV+p2na52DJua24wRRFEkFqOMurYQohi/L5+Za8FKIiJjMfb7vPPbdB4oajsSqRne8r1Mfs
bHofS3fRrz9YSG51OAl0lEirrDgDOXLHR+B5dnZEQfWdkbYZnuZgvQ1v/xdyRThQkPjrVM8=


More information about the bazaar mailing list