On Mon, Apr 14, 2008 at 8:22 PM, Guido van Rossum <guido@python.org> wrote:

Eek! Please use the bug tracker.

I 've made some comments on: http://bugs.python.org/issue1092502 (which is the original issue). However I cannot reopen this issue. Regards, - Ralf

On Mon, Apr 14, 2008 at 11:10:12AM -0700, Curt Hagenlocher wrote:

while True: left = size - buf_len ! recv_size = max(self._rbufsize, left) data = self._sock.recv(recv_size)

What version is this patch against? (The last 2.5 release, maybe?) The max() in the above line should be min(), because you want to use the *smaller* number of the buffer size and the # of remaining bytes to read, not the *larger*. This code is using min() in both 25-maint and trunk. --amk

On Mon, Apr 14, 2008 at 8:10 PM, Curt Hagenlocher <curt@hagenlocher.org> wrote:

On Mon, Apr 14, 2008 at 9:12 AM, Ralf Schmitt <schmir@gmail.com> wrote:

...

I've tracked it down to this change: http://hgpy.de/py/release25-maint/rev/e9446c6ab3cd this is svn revision 61009. [...] self._rbufsize if 1, and so the code reads one byte at a time

The change is correct, but exposes a flaw earlier in the same method. "_rbufsize == 1" represents a request to buffer "by line", which is clearly irrelevant in this context. A request to read n bytes should just use the default buffer size if buffering "by line". Sample patch is attached.

Sorry to reply on the mailing list. But this change is wrong. e.g. if you're using a buffer size of 16 bytes and try to read 256 bytes, it should call recv with a value of 256 and not call recv 16 times with a value of 16. However, there should be an upper limit (as shown by the imap bug). Regards, - Ralf

On Tue, Apr 15, 2008 at 12:19 AM, Curt Hagenlocher <curt@hagenlocher.org> wrote:

On Mon, Apr 14, 2008 at 2:29 PM, Ralf Schmitt <schmir@gmail.com> wrote:

...

Sorry to reply on the mailing list. But this change is wrong. e.g. if you're using a buffer size of 16 bytes and try to read 256

bytes, it

...

should call recv with a value of 256 and not call recv 16 times with a value of 16. However, there should be an upper limit (as shown by the imap bug).

There is an upper limit. It's called "the buffer size". If someone specifies a buffer size of 16 bytes, it means "read 16 bytes at a time". I don't know why someone would want such a small buffer size, but presumably they have their reasons.

No, I don't agree. To me buffer size means buffer up to buffer_size bytes in memory. It does not mean that it should read only buffer_size bytes at once when asked to read more bytes than buffer size. The upper limit I was talking about is the buffer size limit of the operating system, i.e. the operating system will at a maximum return N bytes from recv call. It doesn't make sense to ask for more then, and the original problem with imaplip asking for 10MB of data and then realloc'ing that buffer would be gone.

The only reason "min" is a problem is that there's standard library code passing a zero to socket.makefile, which gets turned into a bufsize of 1 by the constructor. I actually agree with Bill Janssen -- __init__ is where the real problem lies. But I think the change to read() is safer.

again no, if I pass in 4 as buffer size, I don't expect the system to make 1024 calls to recv when I want to read 4096 bytes. Regards, - Ralf

On Tue, Apr 15, 2008 at 1:19 AM, Guido van Rossum <guido@python.org> wrote:

But why was imaplib apparently specifying 10MB? Did it know there was that much data? Or did it just not want to bother looping over all the data in smaller buffer increments (e.g. 64K, which is probably the max of what most TCP stacks will give you)?

Well, calling read with a size of 10MB should be possible. The problem is that this value ended up inside the recv call, which then did the malloc/realloc calls.

If I'm right with my hunch that the TCP stack will probably clamp at 64K, perhaps we should use min(system limit, max(requested size, buffer size))?

yes, this is what I was trying to explain.

-- --Guido van Rossum (home page: http://www.python.org/~guido/<http://www.python.org/%7Eguido/> )

On Mon, Apr 14, 2008 at 4:41 PM, Curt Hagenlocher <curt@hagenlocher.org> wrote:

On Mon, Apr 14, 2008 at 4:19 PM, Guido van Rossum <guido@python.org> wrote:

...

But why was imaplib apparently specifying 10MB? Did it know there was that much data? Or did it just not want to bother looping over all the data in smaller buffer increments (e.g. 64K, which is probably the max of what most TCP stacks will give you)?

I'm going to guess that the code in question is

size = int(self.mo.group('size')) if __debug__: if self.debug >= 4: self._mesg('read literal size %s' % size) data = self.read(size)

It's reading however many bytes are reported by the server as the size.

...

If I'm right with my hunch that the TCP stack will probably clamp at 64K, perhaps we should use min(system limit, max(requested size, buffer size))?

I have indeed missed the point of the read buffer size. This would work.

The 64K hunch is wrong. The system limit can be found using getsockopt(...SO_RCVBUF...). It can easily be (and often is) set to many megabytes either at a system default level or on a per socket level by the user using setsockopt. When the system default is that large, limiting by the system limit would not help the 10mb read case. Even smaller allocations like 64K cause problems as mentioned in issue 1092502 linking to this twisted http://twistedmatrix.com/trac/ticket/1079bug. twisted's solution was to make the string object returned by a recv as short lived as possible by copying it into a StringIO. We could do the same in _fileobject.read() and readline(). I have attached a patch to issue 2632 that changes socket to use StringIO for its read buffer and keeps the lifetime of strings returned by recv() as short as possible when appropriate. It also refuses to call recv() with a size smaller than default_bufsize within read() [the source of the performance problem]. That changes internal recv() call behavior over the existing code after the issue 1092502 "fix" was applied to use min() rather than max(), but it is -not- a significant change over the pre-1092502 "fix" behavior that exists in all released versions of python (it already chose the larger of two values for recv sizes). The main difference behind the scenes? StringIO is using realloc only to increase its size while recv() was using realloc to shrink the allocation size and many of these recv()ed shrunken strings were being held onto in a list before the final value was constructed. I suggest continuing the discussion within issue 2632 to keep better track of it. My socket-strio patch in 2632 needs more testing (it passed socket, http* and url* tests) and verification that both issue's problems are indeed gone but they should be. -gps