Fix a bug introduced in r62627. see issue2760 and issue2632.

gpshead · gpshead · commit 24237ea8a128 · 2008-05-05T21:53:45.000Z
An assertion in readline() would fail as data was already in the
internal buffer even though the socket was in unbuffered read mode.
That case is now handled.  More importantly, read() has been fixed to
not over-recv() and leave newly recv()d data in the _fileobject buffer.

The max() vs min() issue in read() is now gone.  Neither was correct.
On bounded reads, always ask recv() for the exact amount of data we
still need.

Candidate for backporting to release25-maint along with r62627.
diff --git a/Lib/socket.py b/Lib/socket.py
@@ -312,7 +312,8 @@ def _get_wbuf_len(self):
 
     def read(self, size=-1):
         # Use max, disallow tiny reads in a loop as they are very inefficient.
-        # We never leave read() with any leftover data in our internal buffer.
+        # We never leave read() with any leftover data from a new recv() call
+        # in our internal buffer.
         rbufsize = max(self._rbufsize, self.default_bufsize)
         # Our use of StringIO rather than lists of string objects returned by
         # recv() minimizes memory usage and fragmentation that occurs when
@@ -342,13 +343,12 @@ def read(self, size=-1):
             self._rbuf = StringIO()  # reset _rbuf.  we consume it via buf.
             while True:
                 left = size - buf_len
-                # Using max() here means that recv() can malloc a
-                # large amount of memory even though recv may return
-                # much less data than that.  But the returned data
-                # string is short lived in that case as we copy it
-                # into a StringIO and free it.
-                recv_size = max(rbufsize, left)
-                data = self._sock.recv(recv_size)
+                # recv() will malloc the amount of memory given as its
+                # parameter even though it often returns much less data
+                # than that.  The returned data string is short lived
+                # as we copy it into a StringIO and free it.  This avoids
+                # fragmentation issues on many platforms.
+                data = self._sock.recv(left)
                 if not data:
                     break
                 n = len(data)
@@ -359,13 +359,11 @@ def read(self, size=-1):
                     # - Our call to recv returned exactly the
                     #   number of bytes we were asked to read.
                     return data
-                if n >= left:
-                    # avoids data copy of: buf.write(data[:left])
-                    buf.write(buffer(data, 0, left))
-                    # avoids data copy of: self._rbuf.write(data[left:])
-                    self._rbuf.write(buffer(data, left))
+                if n == left:
+                    buf.write(data)
                     del data  # explicit free
                     break
+                assert n <= left, "recv(%d) returned %d bytes" % (left, n)
                 buf.write(data)
                 buf_len += n
                 del data  # explicit free
@@ -374,22 +372,23 @@ def read(self, size=-1):
 
     def readline(self, size=-1):
         buf = self._rbuf
-        if self._rbufsize > 1:
-            # if we're buffering, check if we already have it in our buffer
+        buf.seek(0, 2)  # seek end
+        if buf.tell() > 0:
+            # check if we already have it in our buffer
             buf.seek(0)
             bline = buf.readline(size)
             if bline.endswith('\n') or len(bline) == size:
                 self._rbuf = StringIO()
                 self._rbuf.write(buf.read())
                 return bline
             del bline
-        buf.seek(0, 2)  # seek end
         if size < 0:
             # Read until \n or EOF, whichever comes first
             if self._rbufsize <= 1:
                 # Speed up unbuffered case
-                assert buf.tell() == 0
-                buffers = []
+                buf.seek(0)
+                buffers = [buf.read()]
+                self._rbuf = StringIO()  # reset _rbuf.  we consume it via buf.
                 data = None
                 recv = self._sock.recv
                 while data != "\n":
@@ -399,7 +398,6 @@ def readline(self, size=-1):
                     buffers.append(data)
                 return "".join(buffers)
 
-            buf = self._rbuf
             buf.seek(0, 2)  # seek end
             self._rbuf = StringIO()  # reset _rbuf.  we consume it via buf.
             while True:
@@ -417,6 +415,7 @@ def readline(self, size=-1):
             return buf.getvalue()
         else:
             # Read until size bytes or \n or EOF seen, whichever comes first
+            buf.seek(0, 2)  # seek end
             buf_len = buf.tell()
             if buf_len >= size:
                 buf.seek(0)
diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
@@ -789,6 +789,33 @@ def _testReadline(self):
         self.cli_file.write(MSG)
         self.cli_file.flush()
 
+    def testReadlineAfterRead(self):
+        a_baloo_is = self.serv_file.read(len("A baloo is"))
+        self.assertEqual("A baloo is", a_baloo_is)
+        _a_bear = self.serv_file.read(len(" a bear"))
+        self.assertEqual(" a bear", _a_bear)
+        line = self.serv_file.readline()
+        self.assertEqual("\n", line)
+        line = self.serv_file.readline()
+        self.assertEqual("A BALOO IS A BEAR.\n", line)
+        line = self.serv_file.readline()
+        self.assertEqual(MSG, line)
+
+    def _testReadlineAfterRead(self):
+        self.cli_file.write("A baloo is a bear\n")
+        self.cli_file.write("A BALOO IS A BEAR.\n")
+        self.cli_file.write(MSG)
+        self.cli_file.flush()
+
+    def testReadlineAfterReadNoNewline(self):
+        end_of_ = self.serv_file.read(len("End Of "))
+        self.assertEqual("End Of ", end_of_)
+        line = self.serv_file.readline()
+        self.assertEqual("Line", line)
+
+    def _testReadlineAfterReadNoNewline(self):
+        self.cli_file.write("End Of Line")
+
     def testClosedAttr(self):
         self.assert_(not self.serv_file.closed)
 
