Don't catch AssertionError
robey at lag.net
Wed Dec 27 22:01:48 GMT 2006
[cross-posting to paramiko]
On 27 Dec 2006, at 0:52, John Arbash Meinel wrote:
> Robey Pointer wrote:
>> On 21 Dec 2006, at 15:08, John Arbash Meinel wrote:
>>> I just wanted to remind everyone that if you find yourself writing a
>>> test that catches AssertionError, it is usually a sign that
>>> something is
>>> wrong with the code.
>>> I know of one place where we *have* to. But it is because
>>> paramiko has
>>> an assert, rather than something we are doing.
>> I just grepped bzrlib for this and I think the assert was in
>> transport/sftp.py, not paramiko, so we can easily avoid doing it.
>> Here's a quick bundle that replaces the (commented-out) assert with a
>> PathNotChild exception, and uncomments the corresponding unit tests.
>> The tests all still pass.
> Well, you are correct that this can be changed as you say it is.
> But my comment was regarding around line 455 in sftp_file.py:
> The 'assert length == len(data)' is making sure that the returned data
> is the exact same length as what was requested. Which could be
> if you allowed requests >32KB, or if there was a readv for 100 bytes,
> and there is only 80 bytes left in the file.
> paramiko shouldn't be using 'assert' to check an invariant in
> that it is reading from the real world. It should be doing a real
> and raising a real error if it fails.
Hmmm... Requests over 32K should be caught elsewhere, but it's true
that EOF could cause a short response. Originally the assert was
meant to verify that the readv tables were internally consistent, but
I guess there are other checks on that now (plus unit tests, of course).
I'll remove that assert.
More information about the bazaar