Paramiko throws EOFError rather than returning a 0 length file or ENOENT

[Vincent Ladeuil]

>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:

    jam> Robey Pointer wrote:
    >> 
    >> On 12 Sep 2006, at 8:06, John Arbash Meinel wrote:
    >> 

    jam> ...

    >>> I think it is just a question of what should a file raise
    >>> if it gets truncated. A standard file just returns a
    >>> short read, but no exception.  Would bzr handle that
    >>> properly? Or do we really prefer getting a Disconnected
    >>> error.
    >>> 
    >>> I'm tempted to say Disconnected, because I don't know
    >>> what a short-read would really do. I *think* it would be
    >>> safe, as long as the next action was Disconnected. The
    >>> readv stuff doesn't actually assert that the lengths it
    >>> reads are correct. It should, though.
    >> 
    >> I agree; I don't think that kind of error should be
    >> hidden.  It could lead to silent corruption.
    >> 

    jam> I looked into it closer (see my [merge] requests for
    jam> patches)

    jam> But it turns out that paramiko uses an 'assert' to do a
    jam> safety check, and I can easily add something similar for
    jam> the other code.

    >> 
    >>> On that note, should it keep trying to read more until it
    >>> gets the full length, and fail only if it gets a 0-length
    >>> read, or should it just fail on the first wrong-length
    >>> read?
    >> 
    >> As Vincent points out, a short read isn't necessarily an
    >> error, especially over sockets.  Paramiko tries to hide
    >> this but it's probably still an issue for http, for
    >> example.

Yes it can. But I was a bit vague, I'll try to clarify for the
urllib2/httplib context which happen to be *our* case:

- httplib implements a pipelining of request/responses, so in
  theory, you could issue a second request while processing the
  first response, that means you should guard, not only against
  short reads but also against too long reads (i.e. you MUST not
  read part of the second response),

- urllib2 simplify the problem by requiring the server to close
  the connection at each request, hence, no risk of reading a
  second response (this is what _urllib does today for bzr).

But we can, in theory, have short reads if the server is... let's
say slow or slowed, whatever, it didn't send all the packets we
are waiting for.  But as of today, bzr/_urllib issues only read()
(i.e. no length, gimme all you can) on *blocking* sockets, so in
practice, short reads seem to never occur even if they are
possible. By contrast, the test suite use a non-blocking socket
for the server and indeed, from to time to time, I observe test
failing randomly (more often on my mac) for causes linked to
sockets (Unfortunately, I can't reproduce them at will).

Until we are able to correctly assert the length of the message
body we are waiting for, for *all* cases, I think the safer it to
handle short reads as server failures, close the connection and
reconnect.

Sharing the connection for several requests re-open the too long
read problem )even if we can guarantee to issue only one request
at a time, we may, wrongly (as in bug), try to read too much, but
really, the solution is the same: assert the length of the
message body we are waiting for, for *all* cases and read that,
all of that, but only that, otherwise we will not be able to
handle the next response.

Then, when a short read occurs, we can issue another blocking
read and peacefully waits for the server *or* a lower-level
timeout which will clearly indicate a server death (at least as
far as our request is concerned) then reconnect and re-issue the
*same* request taking care of file-like object position if
needed (see what I mean John? :).

    >>
    >> 
    >> I believe an error like "server disconnected" should always be reported
    >> as some kind of exception (at the API level).

Or handled transparently by a reconnect, but I can't speak for
ftp, may be you don't have a context as well defined as http with
the request.

    jam> Well, at the moment we wrap the result of HTTP in
    jam> another file-like object (cStringIO), because urllib
    jam> files don't have tell or seek. And for ranged requests,
    jam> we just read in the entire contents, and parse it into a
    jam> RangeFile object.

    jam> In time, I'd like to get rid of RangeFile, and work
    jam> similarly to sftp/local, which parses the return
    jam> requests on the fly, rather than buffering the whole
    jam> contents.

    jam> I'd also be happier if we didn't have to buffer the
    jam> whole contents of a plain get() in memory, but the best
    jam> alternative I can think of is to use a disk cache, which
    jam> we don't really want to do either.

I hope I have clarified why it can't be done today and how I want
to achieve that.

    jam> Also, HTTP.readv() also has asserts in there to check
    jam> the size of the returned content. My other patch tries
    jam> to upgrade these things to a real Exception that can be
    jam> caught.

Thanks, that helps :)

        Vincent