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

[Andrew Bennetts]

Eric Holmberg wrote:
[...]
> 
> More comments below on the test -- but if we leave the TCP/IP code in
> the system, is there a better way to find an available port -- I think
> Idle actually tries random ports.

Listen on port 0.  The OS will find a free port for you if you do that.  You
should be able to find examples of doing this elsewhere in the test suite.

[...]
> 
> I'm not familiar with test.http_response.  Could you point me to some
> information on it?
> 
> Does test.http_response.ReadSocket actually send data over the real
> TCP/IP stack?  This test was designed to do that to verify that the
> dreaded MemoryError exception doesn't occur with 1x and 3x the block
> size.

I'm not sure we want to try provoke the MemoryError from recv in our test suite.
I suspect the exact conditions to trigger it don't just depend on platform, or
platform version, but also what else is running on that system (and how much RAM
it has) at the time.  i.e. I wouldn't be surprised if a system with more free
RAM had its kernel internally allocate more buffer space.

So it seems hard to me to reliably trigger the recv error, or even know if we
can trigger the error.  So I'm ok with not writing an automated test for that.

> Without running the data through the TCP/IP stack, I don't see why I'm
> bothering to write these tests since the pre-existing tests already
> provide code-coverage of the code that I have added.

Because _read_limited is only being covered indirectly.  There's no explicit
tests making sure that it never tries to read more than _read_limited_buf_size
at a time.  Although every line of code is getting used by the existing tests,
I don't think this aspect of the logic is actually being checked.  If I replace
“min(size, self._read_limited_buf_size)” with just “size” in your patch, I don't
get any failures (apart from 'Address already in use', which I get with an
unmodified patch)

Also, testing that it only reads at most _read_limited_buf_size at a time is
something that can best be tested directly, without intervening layers (like
HTTP) getting in the way.  Although you may need to make _read_limited a
function rather than a method of RangeFile to make it easy to unit test directly
like that.

I don't think we should be striving for utter perfection here, as the code is
fairly simple.  A single test that "_read_limited" for some large number (e.g.
1.5x the limit) will call "read" on the underlying file-like object with smaller
numbers would be ample, I think.

> > As you see, Aaron and the reviewer of the code you're 
> > patching (I think it was Andrew) were unhappy with the '-1' 
> > usage (inherited from socket.py). IMHO, others will also find 
> > it hard to understand. This comment was added on reviewer 
> > request already, let's try to fix it right so that further 
> > reviews don't have to mention it again.
> 
> I really don't understand what you and Aaron want.  All I did was add a
> small patch to the code and now it seems like you guys want me to fix
> all sorts of things in the surrounding code.  Here are the snippets of
> code.  Make the changes and I'll put them into the code for you.

FWIW, I didn't mind the use of -1, which is a convention I'm used to seeing in
read methods.  So I think Vincent is misattributing that objection.

-Andrew.