[MERGE] [Bug 215426] Re: MemoryError - recv for HTTPthroughProxy

[Eric Holmberg]

Hi Andrew,

I'm putting together another email with the new patch, but just wanted
to respond to your email (see inline responses). 

> 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.

A nice, simple approach :)  For the in-use error, I had to set the
SO_REUSEADDR option for the server socket since it was in shut-down mode
and Linux wouldn't allow immediate re-use (Windows did allow it).
Actual code was sock_server.setsockopt(socket.SOL_SOCKET,
socket.SO_REUSEADDR, 1).

That said, I've actually removed the socket code and switched to mock
objects instead per everybody's advice.


> > 
> > 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.

Yes, that is very true.  Even on the Windows systems that I got the
problem to occur on, the failure point was dependent upon the load on
the TCP/IP stack.  I could run wget while the test was running and get
the problem to occur at a lower socket.recv read size.  Under a Linux
virtual machine, I wasn't able to get the failure to occur, but to be
fair to Windows, I didn't have the same services (NTLM proxy client,
Pylons app) running under Linux.  Glaring conclusion is that this
definitely is not something that we want to test to failure.


> > 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.

Point taken -- I'll go on my test results of large block sizes in the
range of 2 MB to 4 MB cause failures on some machines and set a
conservative 512KB limit for the block size. . . And then just test the
block-size functionality.  Thinking about it more, if I use the TCP/IP
stack for the test, I really need to verify that I can get both a
successful run and a failure run to be sure that the tests are working,
so the mock object test is going to be the best approach.


> 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.
> 

<sigh>  Reality always seems to get in the way of perfection :P  I added
three tests -- two of them handle testing of the different code paths
for read to EOF and read N bytes and a third tests the sub-cases of read
N bytes where N is <, =, and > than the maximum block size.


Regards,

Eric Holmberg