paramiko.sftp_file.prefetch now backwards compatible #677

Closed
wants to merge 3 commits into
from

Projects

None yet

3 participants

@stevevanhooser

This changes paramiko.sftp_file.prefetch so that it can accept 1 or 2 arguments for backwards compatibility.

In versions 1.5 - 1.15, paramiko.sftp_file.prefetch accepted only 1 argument (self), and it was assumed that the entire file would be read in. In the original 1.16 version, the file_size to be read in was obligated (one could not use a default argument). This caused a lack of backwards compatibility with older programs (such as bzr). Now the second input argument is optional; if it is not provided, then the file_size is read directly from the file. Otherwise, the user-provided input argument is used for the file_size.

@stevevanhooser

Note: The travis-cl build fails on Python 3.5 but for code that I didn't modify

@opoplawski

Any progress here? I'm contemplating updating paramiko from 1.15.1 to 1.16.0 in EPEL7, but am trying to avoid API changes.

@bitprophet
Member
bitprophet commented Apr 25, 2016 edited

Thanks for the report & patch, @stevevanhooser - offhand the change under discussion strikes me as an oversight/mistake, re: its effect on backwards compatibility, so I like what I see here. Going to quickly review the original changes/discussion to make sure there wasn't some angle I am forgetting, then drop this in for the upcoming bugfix release (1.16.1).

@bitprophet bitprophet added the Bug label Apr 25, 2016
@bitprophet bitprophet added this to the 1.16.1 milestone Apr 25, 2016
@bitprophet
Member
bitprophet commented Apr 25, 2016 edited

OK, I'm glad I looked into it, this patch is effectively undoing the earlier change, which very specifically moved the stat call out of prefetch: a671daf (from #562).

So we need to figure out how to square these two concerns. Unless there's some other way to prevent the bug #562 fixes (certainly possible), I suspect we need to push the burden of this on users like bzr, asking them to perform the stat on their own (maybe we can cut the difference with a convenience wrapper? e.g. .prefetch_all()). I.e. how the test suite updates in #562 all added the external stat call.

Bumping this to 1.16.2 pending a discussion.

/cc the principals affected by the original bug #562 fixes: @torkil, @muraleee, @ryanahall.

@bitprophet bitprophet modified the milestone: 1.16.2, 1.16.1 Apr 25, 2016
@stevevanhooser

Hi all -

The fix here only calls stat in a conditional manner (if size is not specified). Therefore it is backward compatible but any new code can avoid it by passing the size directly. I didn't add any documentation that suggests a preferred method of calling.

I don't know how many old programs or programs that are infrequently updated are impacted. My entire group uses bzr and currently we are using this branch instead of the main paramiko distribution. (Some are non-programmers so it is a long process for them.)

/cc the principals affected by the original bug #562 fixes: @torkil, @muraleee, @ryanahall, and a user that asked for status @opoplawski.

@bitprophet
Member

Thanks for clarifying, @stevevanhooser - that's a good point & one I had overlooked (the perils of sprinting...). As far as I can tell, the original bug was predicated on Paramiko's internal use of prefetch (from getfo, in turn from get), so yea - your change shouldn't cause those call chains to resurface the bug.

I think I'll add a bit to prefetch's docstring noting the potential for the bug if a user calls it directly, and that should hopefully cover all bases.

@bitprophet bitprophet added a commit that referenced this pull request Apr 25, 2016
@bitprophet bitprophet Reword/link changelog re #677, re #676 3b8ebc9
@bitprophet
Member

Cherry-picked to the 1.16 branch. Thanks!

@bitprophet bitprophet closed this Apr 25, 2016
@opoplawski

Since this got into 1.16.1, might be nice to change the milestone back. Thanks for fixing this.

@bitprophet bitprophet modified the milestone: 1.16.1, 1.16.2 May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment