Rev 3568: Make sure we never read more than 64k at a time from a smart medium. in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Tue Jul 22 02:25:08 BST 2008
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 3568
revision-id:pqm at pqm.ubuntu.com-20080722012453-i58b5mk2wayinusg
parent: pqm at pqm.ubuntu.com-20080721151553-11iasd1407hkznk1
parent: andrew.bennetts at canonical.com-20080722005803-ldnjji030xujgz6g
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Tue 2008-07-22 02:24:53 +0100
message:
Make sure we never read more than 64k at a time from a smart medium.
(Andrew Bennetts, #246180)
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
bzrlib/smart/medium.py medium.py-20061103051856-rgu2huy59fkz902q-1
bzrlib/smart/protocol.py protocol.py-20061108035435-ot0lstk2590yqhzr-1
bzrlib/transport/http/__init__.py http_transport.py-20050711212304-506c5fd1059ace96
------------------------------------------------------------
revno: 3565.1.4
revision-id:andrew.bennetts at canonical.com-20080722005803-ldnjji030xujgz6g
parent: andrew.bennetts at canonical.com-20080722005219-0r7fsjq3saiway80
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: hpss-v3-memory-error
timestamp: Tue 2008-07-22 10:58:03 +1000
message:
Add NEWS entry.
modified:
NEWS NEWS-20050323055033-4e00b5db738777ff
------------------------------------------------------------
revno: 3565.1.3
revision-id:andrew.bennetts at canonical.com-20080722005219-0r7fsjq3saiway80
parent: andrew.bennetts at canonical.com-20080721084517-cr5489aq0kb9zw5z
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: hpss-v3-memory-error
timestamp: Tue 2008-07-22 10:52:19 +1000
message:
Define a _MAX_READ_SIZE constant as suggested by John's review.
modified:
bzrlib/smart/medium.py medium.py-20061103051856-rgu2huy59fkz902q-1
------------------------------------------------------------
revno: 3565.1.2
revision-id:andrew.bennetts at canonical.com-20080721084517-cr5489aq0kb9zw5z
parent: andrew.bennetts at canonical.com-20080721042421-63lh85e76o57jch4
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: hpss-v3-memory-error
timestamp: Mon 2008-07-21 18:45:17 +1000
message:
Delete some more code, fix some bugs, add more comments.
modified:
bzrlib/smart/medium.py medium.py-20061103051856-rgu2huy59fkz902q-1
bzrlib/transport/http/__init__.py http_transport.py-20050711212304-506c5fd1059ace96
------------------------------------------------------------
revno: 3565.1.1
revision-id:andrew.bennetts at canonical.com-20080721042421-63lh85e76o57jch4
parent: pqm at pqm.ubuntu.com-20080718100017-segv2csk7ux2xs9p
committer: Andrew Bennetts <andrew.bennetts at canonical.com>
branch nick: hpss-v3-memory-error
timestamp: Mon 2008-07-21 14:24:21 +1000
message:
Read no more then 64k at a time in the smart protocol code.
The logic for this has been moved entirely into bzrlib.smart.medium, and
duplication (both in that module, and in bzrlib.smart.protocol) has been mostly
refactored out. In particular there's now a SmartMedium base class used for
both client- and server-side media, and only one place that reading a line is
implemented.
modified:
bzrlib/smart/medium.py medium.py-20061103051856-rgu2huy59fkz902q-1
bzrlib/smart/protocol.py protocol.py-20061108035435-ot0lstk2590yqhzr-1
=== modified file 'NEWS'
--- a/NEWS 2008-07-21 14:46:05 +0000
+++ b/NEWS 2008-07-22 01:24:53 +0000
@@ -27,6 +27,10 @@
* Fix a test case that was failing if encoding wasn't UTF-8.
(John Arbash Meinel, #247585)
+ * Fix "no buffer space available" error when branching with the new
+ smart server protocol to or from Windows.
+ (Andrew Bennetts, #246180)
+
DOCUMENTATION:
=== modified file 'bzrlib/smart/medium.py'
--- a/bzrlib/smart/medium.py 2008-07-13 16:45:14 +0000
+++ b/bzrlib/smart/medium.py 2008-07-22 00:52:19 +0000
@@ -42,6 +42,12 @@
""")
+# We must not read any more than 64k at a time so we don't risk "no buffer
+# space available" errors on some platforms. Windows in particular is likely
+# to give error 10053 or 10055 if we read more than 64k from a socket.
+_MAX_READ_SIZE = 64 * 1024
+
+
def _get_protocol_factory_for_bytes(bytes):
"""Determine the right protocol factory for 'bytes'.
@@ -74,7 +80,72 @@
return protocol_factory, bytes
-class SmartServerStreamMedium(object):
+class SmartMedium(object):
+ """Base class for smart protocol media, both client- and server-side."""
+
+ def __init__(self):
+ self._push_back_buffer = None
+
+ def _push_back(self, bytes):
+ """Return unused bytes to the medium, because they belong to the next
+ request(s).
+
+ This sets the _push_back_buffer to the given bytes.
+ """
+ if self._push_back_buffer is not None:
+ raise AssertionError(
+ "_push_back called when self._push_back_buffer is %r"
+ % (self._push_back_buffer,))
+ if bytes == '':
+ return
+ self._push_back_buffer = bytes
+
+ def _get_push_back_buffer(self):
+ if self._push_back_buffer == '':
+ raise AssertionError(
+ '%s._push_back_buffer should never be the empty string, '
+ 'which can be confused with EOF' % (self,))
+ bytes = self._push_back_buffer
+ self._push_back_buffer = None
+ return bytes
+
+ def read_bytes(self, desired_count):
+ """Read some bytes from this medium.
+
+ :returns: some bytes, possibly more or less than the number requested
+ in 'desired_count' depending on the medium.
+ """
+ if self._push_back_buffer is not None:
+ return self._get_push_back_buffer()
+ bytes_to_read = min(desired_count, _MAX_READ_SIZE)
+ return self._read_bytes(bytes_to_read)
+
+ def _read_bytes(self, count):
+ raise NotImplementedError(self._read_bytes)
+
+ def _get_line(self):
+ """Read bytes from this request's response until a newline byte.
+
+ This isn't particularly efficient, so should only be used when the
+ expected size of the line is quite short.
+
+ :returns: a string of bytes ending in a newline (byte 0x0A).
+ """
+ newline_pos = -1
+ bytes = ''
+ while newline_pos == -1:
+ new_bytes = self.read_bytes(1)
+ bytes += new_bytes
+ if new_bytes == '':
+ # Ran out of bytes before receiving a complete line.
+ return bytes
+ newline_pos = bytes.find('\n')
+ line = bytes[:newline_pos+1]
+ self._push_back(bytes[newline_pos+1:])
+ return line
+
+
+class SmartServerStreamMedium(SmartMedium):
"""Handles smart commands coming over a stream.
The stream may be a pipe connected to sshd, or a tcp socket, or an
@@ -101,30 +172,7 @@
self.backing_transport = backing_transport
self.root_client_path = root_client_path
self.finished = False
- self._push_back_buffer = None
-
- def _push_back(self, bytes):
- """Return unused bytes to the medium, because they belong to the next
- request(s).
-
- This sets the _push_back_buffer to the given bytes.
- """
- if self._push_back_buffer is not None:
- raise AssertionError(
- "_push_back called when self._push_back_buffer is %r"
- % (self._push_back_buffer,))
- if bytes == '':
- return
- self._push_back_buffer = bytes
-
- def _get_push_back_buffer(self):
- if self._push_back_buffer == '':
- raise AssertionError(
- '%s._push_back_buffer should never be the empty string, '
- 'which can be confused with EOF' % (self,))
- bytes = self._push_back_buffer
- self._push_back_buffer = None
- return bytes
+ SmartMedium.__init__(self)
def serve(self):
"""Serve requests until the client disconnects."""
@@ -171,34 +219,13 @@
"""Called when an unhandled exception from the protocol occurs."""
raise NotImplementedError(self.terminate_due_to_error)
- def _get_bytes(self, desired_count):
+ def _read_bytes(self, desired_count):
"""Get some bytes from the medium.
:param desired_count: number of bytes we want to read.
"""
- raise NotImplementedError(self._get_bytes)
-
- def _get_line(self):
- """Read bytes from this request's response until a newline byte.
-
- This isn't particularly efficient, so should only be used when the
- expected size of the line is quite short.
-
- :returns: a string of bytes ending in a newline (byte 0x0A).
- """
- 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.
- return bytes
- newline_pos = bytes.find('\n')
- line = bytes[:newline_pos+1]
- self._push_back(bytes[newline_pos+1:])
- return line
-
+ raise NotImplementedError(self._read_bytes)
+
class SmartServerSocketStreamMedium(SmartServerStreamMedium):
@@ -215,7 +242,10 @@
def _serve_one_request_unguarded(self, protocol):
while protocol.next_read_size():
- bytes = self._get_bytes(4096)
+ # We can safely try to read large chunks. If there is less data
+ # than _MAX_READ_SIZE ready, the socket wil just return a short
+ # read immediately rather than block.
+ bytes = self.read_bytes(_MAX_READ_SIZE)
if bytes == '':
self.finished = True
return
@@ -223,13 +253,11 @@
self._push_back(protocol.unused_data)
- def _get_bytes(self, desired_count):
- if self._push_back_buffer is not None:
- return self._get_push_back_buffer()
+ def _read_bytes(self, desired_count):
# We ignore the desired_count because on sockets it's more efficient to
- # read 4k at a time.
- return self.socket.recv(4096)
-
+ # read large chunks (of _MAX_READ_SIZE bytes) at a time.
+ return self.socket.recv(_MAX_READ_SIZE)
+
def terminate_due_to_error(self):
# TODO: This should log to a server log file, but no such thing
# exists yet. Andrew Bennetts 2006-09-29.
@@ -262,12 +290,15 @@
def _serve_one_request_unguarded(self, protocol):
while True:
+ # We need to be careful not to read past the end of the current
+ # request, or else the read from the pipe will block, so we use
+ # protocol.next_read_size().
bytes_to_read = protocol.next_read_size()
if bytes_to_read == 0:
# Finished serving this request.
self._out.flush()
return
- bytes = self._get_bytes(bytes_to_read)
+ bytes = self.read_bytes(bytes_to_read)
if bytes == '':
# Connection has been closed.
self.finished = True
@@ -275,9 +306,7 @@
return
protocol.accept_bytes(bytes)
- def _get_bytes(self, desired_count):
- if self._push_back_buffer is not None:
- return self._get_push_back_buffer()
+ def _read_bytes(self, desired_count):
return self._in.read(desired_count)
def terminate_due_to_error(self):
@@ -397,36 +426,28 @@
return self._read_bytes(count)
def _read_bytes(self, count):
- """Helper for read_bytes.
+ """Helper for SmartClientMediumRequest.read_bytes.
read_bytes checks the state of the request to determing if bytes
should be read. After that it hands off to _read_bytes to do the
actual read.
+
+ By default this forwards to self._medium.read_bytes because we are
+ operating on the medium's stream.
"""
- raise NotImplementedError(self._read_bytes)
+ return self._medium.read_bytes(count)
def read_line(self):
- """Read bytes from this request's response until a newline byte.
-
- This isn't particularly efficient, so should only be used when the
- expected size of the line is quite short.
-
- :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.read_bytes(1)
- line += new_char
- if new_char == '':
- # end of file encountered reading from server
- raise errors.ConnectionReset(
- "please check connectivity and permissions",
- "(and try -Dhpss if further diagnosis is required)")
+ line = self._medium._get_line()
+ if not line.endswith('\n'):
+ # end of file encountered reading from server
+ raise errors.ConnectionReset(
+ "please check connectivity and permissions",
+ "(and try -Dhpss if further diagnosis is required)")
return line
-class SmartClientMedium(object):
+class SmartClientMedium(SmartMedium):
"""Smart client is a medium for sending smart protocol requests over."""
def __init__(self, base):
@@ -567,9 +588,6 @@
"""
return SmartClientStreamMediumRequest(self)
- def read_bytes(self, count):
- return self._read_bytes(count)
-
class SmartSimplePipesClientMedium(SmartClientStreamMedium):
"""A client medium using simple pipes.
@@ -660,7 +678,8 @@
"""See SmartClientStreamMedium.read_bytes."""
if not self._connected:
raise errors.MediumNotConnected(self)
- return self._read_from.read(count)
+ bytes_to_read = min(count, _MAX_READ_SIZE)
+ return self._read_from.read(bytes_to_read)
# Port 4155 is the default port for bzr://, registered with IANA.
@@ -726,7 +745,9 @@
"""See SmartClientMedium.read_bytes."""
if not self._connected:
raise errors.MediumNotConnected(self)
- return self._socket.recv(count)
+ # We ignore the desired_count because on sockets it's more efficient to
+ # read large chunks (of _MAX_READ_SIZE bytes) at a time.
+ return self._socket.recv(_MAX_READ_SIZE)
class SmartClientStreamMediumRequest(SmartClientMediumRequest):
@@ -768,11 +789,3 @@
"""
self._medium._flush()
- def _read_bytes(self, count):
- """See SmartClientMediumRequest._read_bytes.
-
- This forwards to self._medium._read_bytes because we are operating
- on the mediums stream.
- """
- return self._medium._read_bytes(count)
-
=== modified file 'bzrlib/smart/protocol.py'
--- a/bzrlib/smart/protocol.py 2008-06-02 01:12:17 +0000
+++ b/bzrlib/smart/protocol.py 2008-07-21 04:24:21 +0000
@@ -698,12 +698,8 @@
return self._body_buffer.read(count)
_body_decoder = LengthPrefixedBodyDecoder()
- # Read no more than 64k at a time so that we don't risk error 10055 (no
- # buffer space available) on Windows.
- max_read = 64 * 1024
while not _body_decoder.finished_reading:
- bytes_wanted = min(_body_decoder.next_read_size(), max_read)
- bytes = self._request.read_bytes(bytes_wanted)
+ bytes = self._request.read_bytes(_body_decoder.next_read_size())
if bytes == '':
# end of file encountered reading from server
raise errors.ConnectionReset(
@@ -719,21 +715,7 @@
def _recv_tuple(self):
"""Receive a tuple from the medium request."""
- return _decode_tuple(self._recv_line())
-
- def _recv_line(self):
- """Read an entire line from the medium request."""
- line = ''
- while not line or line[-1] != '\n':
- # TODO: this is inefficient - but tuples are short.
- new_char = self._request.read_bytes(1)
- if new_char == '':
- # end of file encountered reading from server
- raise errors.ConnectionReset(
- "please check connectivity and permissions",
- "(and try -Dhpss if further diagnosis is required)")
- line += new_char
- return line
+ return _decode_tuple(self._request.read_line())
def query_version(self):
"""Return protocol version number of the server."""
@@ -776,7 +758,7 @@
if version != self.response_marker:
self._request.finished_reading()
raise errors.UnexpectedProtocolVersionMarker(version)
- response_status = self._recv_line()
+ response_status = self._request.read_line()
result = SmartClientRequestProtocolOne._read_response_tuple(self)
self._response_is_unknown_method(result)
if response_status == 'success\n':
@@ -804,11 +786,9 @@
"""
# Read no more than 64k at a time so that we don't risk error 10055 (no
# buffer space available) on Windows.
- max_read = 64 * 1024
_body_decoder = ChunkedBodyDecoder()
while not _body_decoder.finished_reading:
- bytes_wanted = min(_body_decoder.next_read_size(), max_read)
- bytes = self._request.read_bytes(bytes_wanted)
+ bytes = self._request.read_bytes(_body_decoder.next_read_size())
if bytes == '':
# end of file encountered reading from server
raise errors.ConnectionReset(
=== modified file 'bzrlib/transport/http/__init__.py'
--- a/bzrlib/transport/http/__init__.py 2008-05-20 00:42:19 +0000
+++ b/bzrlib/transport/http/__init__.py 2008-07-21 08:45:17 +0000
@@ -552,6 +552,7 @@
self._response_body = data
def _read_bytes(self, count):
+ """See SmartClientMediumRequest._read_bytes."""
return self._response_body.read(count)
def _finished_reading(self):
More information about the bazaar-commits
mailing list