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

[Eric Holmberg]

> ^^^ Shouldn't the meaning of -1 be documented?  It's a rather 
> unfortunate, but since it's used this way in the caller, I 
> can tolerate it.
Yes, that should be documented, I'll add that.


> self._file.read(self.__read_limited_buf_size)
> > +                buffer.write(data)
> > +                if len(data) < self.__read_limited_buf_size:
> > +                    # EOF hit
> > +                    break
> 
> ^^^ This looks an awful lot like osutils.pumpfile.  Perhaps 
> we should extend that instead?
Yes, that section of the code does look like osutils.pumpfile with the
exception of the block size.  I don't see any issues with changing the
EOF section to use osutils.pumpfile.  

Should I add a defaulted chunk size parameter to osutils.pumpfile so we
can pass in the __read_limited_buf_size buffer size. . . Or were you
thinking that I would move all of the _read_limited functionality into
osutils.pumpfile?


> Also, it would be nice to have tests of at least everything testable.
> Enhancing osutils.pumpfile would definitely be a nice step in 
> that direction.
The changes that I made should already be covered by the current tests.
Were you looking for a test of the XP issue (Bug 215426) that this
fixes?  If so, I'll have to figure out how to reproduce this reliably
and can then integrate it into bzrlib/tests/test_http_resonse.py.



Just an FYI, we need to update the wiki to provide links to developer's
guide (which will include running unit tests and the coding style) in
the Giving-back page (http://bazaar-vcs.org/BzrGivingBack).  The easier
it is to get going, the more likely people will contribute.


-Eric







 

> -----Original Message-----
> From: Aaron Bentley [mailto:aaron at aaronbentley.com] 
> Sent: Wednesday, April 23, 2008 9:32 PM
> To: Eric Holmberg
> Cc: bazaar at lists.canonical.com; Andrew Bennetts
> Subject: Re: [MERGE] [Bug 215426] Re: MemoryError - recv for 
> HTTP throughProxy
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Eric Holmberg wrote:
> > The updated patch is attached and I've addressed your 
> comments below.
> > Sorry about the delay -- I was on travel last week and didn't get a 
> > chance to update the patch.
> > 
> > 
> > === modified file 'bzrlib/transport/http/response.py'
> > +    def _read_limited(self, size=-1):
> 
> I don't think _read_limited needs a default value-- it's 
> meant to be called only by read, right?
> 
> 
> > +        """Reads size bytes from the _file while limiting the read
> > +        size to 100KB.
> > +        """
> 
> ^^^ Shouldn't the meaning of -1 be documented?  It's a rather 
> unfortunate, but since it's used this way in the caller, I 
> can tolerate it.
> 
> > +        buffer = StringIO()
> > +        
> > +        if size >= 0:        
> > +            # limited read - read specified size in chunks
> > +            while size > 0:
> > +                # limit size of reads as work-around for Windows XP
> > +                # MemoryError problems in recv.
> > +                # See bug: 
> http://bugs.launchpad.net/bzr/+bug/215426 for
> > +                # details
> > +                num_bytes_to_read = 
> > + min(size,self.__read_limited_buf_size)
> 
> ^^^ items in lists should always have a space after the comma.
> 
> > +                buffer.write(self._file.read(num_bytes_to_read))
> > +                size -= num_bytes_to_read
> > +        else:
> > +            # read to EOF in chunks
> > +            while True:
> > +                data = 
> self._file.read(self.__read_limited_buf_size)
> > +                buffer.write(data)
> > +                if len(data) < self.__read_limited_buf_size:
> > +                    # EOF hit
> > +                    break
> 
> ^^^ This looks an awful lot like osutils.pumpfile.  Perhaps 
> we should extend that instead?
> 
> Also, it would be nice to have tests of at least everything testable.
> Enhancing osutils.pumpfile would definitely be a nice step in 
> that direction.
> 
> bb:resubmit
> 
> Aaron
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
> 
> iD8DBQFID/8Y0F+nu1YWqI0RAk1jAJ4qwXs9O6IxvuDMEWYP8SIKDRs61gCfbDUa
> 5Y8BfCCBnqcZ3J/KlDgeMSU=
> =YvWq
> -----END PGP SIGNATURE-----
>