SFTPFile prefetch assumes response order matches requests #34

Closed
ewxrjk opened this Issue Aug 29, 2011 · 2 comments

Projects

None yet

2 participants

@ewxrjk
Contributor
ewxrjk commented Aug 29, 2011

SFTPFile._async_response() assumes that the responses arrive in the same order in which the requests were sent, using a simple queue (self._prefetch_reads). The result is that if SFTPFile is used with an SFTP server which does not guarantee that the response order matches the request order, it gets the ordering of prefetched files wrong.

The SFTP v3 specification is very clear that response order is not required not match request order; in section 6.1 of the -02 draft:

There are no restrictions on the order in which responses to
outstanding requests are delivered to the client, except that the
server must ensure fairness in the sense that processing of no
request will be indefinitely delayed even if the client is sending
other requests so that there are multiple outstanding requests all
the time.

There is a restriction that requests have to be processed in order, i.e. a read following a write must reflet the effect of the write if they overlap, but that's not directly relevant to read-only prefetching.

IMHO the best way to fix this would be to have _async_request pass a closure rather than the SFTPFile object, allowing each request to carry the offset/length details with it, and also thereby decoupling SFTPClient from (this aspect of) SFTPFile's interface.

The reason I noticed is of course that I have a reordering-capable server, though currently reordering is partially disabled, precisely to avoid tickling this bug in Paramiko.

@ewxrjk
Contributor
ewxrjk commented Aug 29, 2011

Huh, I hadn’t realized it would treat the pull request as a separate issue. Sorry about that. Anyway, proposed fix over in pull request #35. A bit different to the discussion here.

@bitprophet
Member

Rolling this into #35.

@bitprophet bitprophet closed this Jan 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment