[MERGE] [Bug 215426] Re: MemoryError - recv for HTTP throughProxy

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eric Holmberg wrote:
| I will make the tweaks and resubmit the patch.  BTW, I did some more
| testing of the patch and limiting the size to 4 MB stills fails on some
| Windows XP clients, so it may be more conservative to lower the max size
| to 1 MB.  I'm not sure what the performance impact is for this.  Also,
| I'm unsure of a proper unit test for this.  Comments are welcome.
|
| Regards,
|
| Eric Holmberg
|
| -----Original Message-----
| From: Andrew Bennetts [mailto:andrew at canonical.com]
| Sent: Wed 16/04/2008 12:39 AM
| To: Eric Holmberg
| Cc: bazaar at lists.canonical.com
| Subject: Re: [MERGE]  [Bug 215426] Re: MemoryError - recv for HTTP
| throughProxy
|
| Eric Holmberg wrote:
|  > Work-around patch for cause of MemoryError on HTTP download under
| Windows is attached.
|
| bb:tweak
|
| Comments inline.
|
|  > === modified file 'bzrlib/transport/http/response.py'
|  > --- bzrlib/transport/http/response.py 2008-01-03 16:26:32 +0000
|  > +++ bzrlib/transport/http/response.py 2008-04-11 21:28:24 +0000
|  > @@ -206,7 +206,18 @@
|  >              limited = self._start + self._size - self._pos
|  >              if size >= 0:
|  >                  limited = min(limited, size)
|  > -            data = self._file.read(limited)
|  > +
|  > +            lst = []
|  > +            while limited > 0:
|  > +                # limit reads to 4 MB for Windows problem
|  > +                # See bug: http://bugs.launchpad.net/bzr/+bug/215426
|  > +                # nBytesToRead = min(limited,1024*1024*4 + 1024*100) OK
|  > +                # nBytesToRead = min(limited,1024*1024*4 + 1024*128)
| Fails
|  > +                nBytesToRead = min(limited,1024*1024*4)
|
| Our style guide says this line ought to be written as:
|
|                 num_bytes_to_read = min(limited, 1024*1024*4)
|
| For that matter, "lst" ought to be called something clearer, like
| "byte_segments".
|
|  > +                lst.append(self._file.read(nBytesToRead))
|  > +                limited -= nBytesToRead
|  > +            data = ''.join(lst)
|
| I wonder a little if a cStringIO would be a faster (and maybe clearer)
| way to
| build up this string?
|
|             buffer = StringIO()
|             while limited > 0:
|                 # limit reads to 4 MB for Windows problem
|                 # See bug: http://bugs.launchpad.net/bzr/+bug/215426
|                 # num_bytes_to_read = min(limited, 1024*1024*4 +
| 1024*100) OK
|                 # num_bytes_to_read = min(limited, 1024*1024*4 +
| 1024*128) Fails
|                 num_bytes_to_read = min(limited, 1024*1024*4)
|                 bytes = self._file.read(num_bytes_to_read)
|                 buffer.write(bytes)
|                 limited -= num_bytes_to_read
|             data = buffer.getvalue()
|
| I don't mind either way, though.
|
| -Andrew.
|
|

I think anything above 100KB is going to be reasonable. The idea is that you
don't want to read 1 byte at a time. I think Capping it at 1MB is ok, but so
would 100KB or so. Probably even 10k might work, but my happy point would
probably be 100K or 1M.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgGG3wACgkQJdeBCYSNAANQbgCgu7ypqiMBqdItI3b2+d/Z2+Q9
c8AAn3D+t55Nbv/Z58cZ2bS9Y7Gld/Ym
=84NH
-----END PGP SIGNATURE-----