From 974294ad7d083c49bffe9189cac0d5b08984b1ad Mon Sep 17 00:00:00 2001 From: Richard Kettlewell Date: Mon, 29 Aug 2011 15:50:35 +0100 Subject: [PATCH 01/10] Fix issue 34 (SFTPFile prefetch assumes response order matches requests) SFTPFile._async_response gets a new 'num' parameter giving the request number. This can be matched up with the return value of SFTPClient._async_request() to retrieve data specific to that request. The prefetch queue SFTPFile._prefetch_reads is replaced with the dict _prefetch_extents, which maps request numbers to (offset,length) tuples. A lock is used to exclude the case where a response arrives in _async_response before _prefetch_thread has updated it. --- paramiko/sftp_client.py | 2 +- paramiko/sftp_file.py | 22 +++++++++++++--------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/paramiko/sftp_client.py b/paramiko/sftp_client.py index 79a7761..189caa4 100644 --- a/paramiko/sftp_client.py +++ b/paramiko/sftp_client.py @@ -682,7 +682,7 @@ class SFTPClient (BaseSFTP): self._convert_status(msg) return t, msg if fileobj is not type(None): - fileobj._async_response(t, msg) + fileobj._async_response(t, msg, num) if waitfor is None: # just doing a single check break diff --git a/paramiko/sftp_file.py b/paramiko/sftp_file.py index 8c5c7ac..2d4d431 100644 --- a/paramiko/sftp_file.py +++ b/paramiko/sftp_file.py @@ -49,7 +49,8 @@ class SFTPFile (BufferedFile): self._prefetching = False self._prefetch_done = False self._prefetch_data = {} - self._prefetch_reads = [] + self._prefetch_extents = {} + self._prefetch_lock = threading.Lock() self._saved_exception = None def __del__(self): @@ -86,7 +87,7 @@ class SFTPFile (BufferedFile): pass def _data_in_prefetch_requests(self, offset, size): - k = [i for i in self._prefetch_reads if i[0] <= offset] + k = [self._prefetch_extents[i] for i in self._prefetch_extents if self._prefetch_extents[i][0] <= offset] if len(k) == 0: return False k.sort(lambda x, y: cmp(x[0], y[0])) @@ -440,7 +441,6 @@ class SFTPFile (BufferedFile): def _start_prefetch(self, chunks): self._prefetching = True self._prefetch_done = False - self._prefetch_reads.extend(chunks) t = threading.Thread(target=self._prefetch_thread, args=(chunks,)) t.setDaemon(True) @@ -450,9 +450,11 @@ class SFTPFile (BufferedFile): # do these read requests in a temporary thread because there may be # a lot of them, so it may block. for offset, length in chunks: - self.sftp._async_request(self, CMD_READ, self.handle, long(offset), int(length)) + with self._prefetch_lock: + num = self.sftp._async_request(self, CMD_READ, self.handle, long(offset), int(length)) + self._prefetch_extents[num] = (offset, length) - def _async_response(self, t, msg): + def _async_response(self, t, msg, num): if t == CMD_STATUS: # save exception and re-raise it on next file operation try: @@ -463,10 +465,12 @@ class SFTPFile (BufferedFile): if t != CMD_DATA: raise SFTPError('Expected data') data = msg.get_string() - offset, length = self._prefetch_reads.pop(0) - self._prefetch_data[offset] = data - if len(self._prefetch_reads) == 0: - self._prefetch_done = True + with self._prefetch_lock: + offset,length = self._prefetch_extents[num] + self._prefetch_data[offset] = data + del self._prefetch_extents[num] + if len(self._prefetch_extents) == 0: + self._prefetch_done = True def _check_exception(self): "if there's a saved exception, raise & clear it" From fe3d3beca4985d7c99a306a269cdbd76091ead6a Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 17:59:25 -0800 Subject: [PATCH 02/10] Fix extra colon in changelog --- sites/www/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 235978d..2378b99 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,7 +2,7 @@ Changelog ========= -* :bug:`193` (and its attentant PRs :issue:`230` & :issue:`253`): Fix SSH agent +* :bug:`193` (and its attentant PRs :issue:`230` & :issue:`253`) Fix SSH agent problems present on Windows. Thanks to David Hobbs for initial report and to Aarni Koskela & Olle Lundberg for the patches. * :release:`1.10.5 <2014-01-08>` From 0a2887a8cca93eb5278ff79ed4b014e0ea24c53c Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 17:59:32 -0800 Subject: [PATCH 03/10] Changelog re #35 --- sites/www/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 2378b99..1a82758 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,9 @@ Changelog ========= +* :bug:`34` (PR :issue:`35`) Fix SFTP prefetching incompatibility with some + SFTP servers regarding request/response ordering. Thanks to Richard + Kettlewell for catch & patch. * :bug:`193` (and its attentant PRs :issue:`230` & :issue:`253`) Fix SSH agent problems present on Windows. Thanks to David Hobbs for initial report and to Aarni Koskela & Olle Lundberg for the patches. From 9fa2cfee9d182eefe918c0303c7966667d9673c9 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 18:03:25 -0800 Subject: [PATCH 04/10] Replace incorrect import of generic test runner w/ custom task --- tasks.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tasks.py b/tasks.py index c716415..e402599 100644 --- a/tasks.py +++ b/tasks.py @@ -1,7 +1,7 @@ from os.path import join -from invoke import Collection -from invocations import docs as _docs, testing +from invoke import Collection, task +from invocations import docs as _docs d = 'sites' @@ -20,4 +20,11 @@ www = Collection.from_module(_docs, name='www', config={ 'sphinx.target': join(path, '_build'), }) -ns = Collection(testing.test, docs=docs, www=www) + +# Until we move to spec-based testing +@task +def test(ctx): + ctx.run("python test.py --verbose") + + +ns = Collection(test, docs=docs, www=www) From aed86f26bff44106b3c564fb29b816a8b5da42b0 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 18:10:52 -0800 Subject: [PATCH 05/10] Future import for with: under py25 --- paramiko/sftp_file.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/paramiko/sftp_file.py b/paramiko/sftp_file.py index b7cf850..ac8ceaf 100644 --- a/paramiko/sftp_file.py +++ b/paramiko/sftp_file.py @@ -20,6 +20,8 @@ L{SFTPFile} """ +from __future__ import with_statement + from binascii import hexlify from collections import deque import socket From 280c240685b6c5d66bde1e81cb160c24c8212387 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 18:11:29 -0800 Subject: [PATCH 06/10] Whitespace --- paramiko/sftp_file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paramiko/sftp_file.py b/paramiko/sftp_file.py index ac8ceaf..c426d6e 100644 --- a/paramiko/sftp_file.py +++ b/paramiko/sftp_file.py @@ -475,7 +475,7 @@ class SFTPFile (BufferedFile): raise SFTPError('Expected data') data = msg.get_string() with self._prefetch_lock: - offset,length = self._prefetch_extents[num] + offset, length = self._prefetch_extents[num] self._prefetch_data[offset] = data del self._prefetch_extents[num] if len(self._prefetch_extents) == 0: From 1d87a0ceb9b6689e3969cc2b528f842b51e68f88 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 18:21:29 -0800 Subject: [PATCH 07/10] Herp and a derp --- tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tasks.py b/tasks.py index e402599..79cb94f 100644 --- a/tasks.py +++ b/tasks.py @@ -1,6 +1,6 @@ from os.path import join -from invoke import Collection, task +from invoke import Collection, ctask as task from invocations import docs as _docs From 675e30986e3aa2882093b8c983a93619f61a019d Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 10 Feb 2014 18:21:47 -0800 Subject: [PATCH 08/10] Clean up a list-comp (also fixes race condition) --- paramiko/sftp_file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paramiko/sftp_file.py b/paramiko/sftp_file.py index c426d6e..a39b1f4 100644 --- a/paramiko/sftp_file.py +++ b/paramiko/sftp_file.py @@ -94,7 +94,7 @@ class SFTPFile (BufferedFile): pass def _data_in_prefetch_requests(self, offset, size): - k = [self._prefetch_extents[i] for i in self._prefetch_extents if self._prefetch_extents[i][0] <= offset] + k = [x for x in self._prefetch_extents.values() if x[0] <= offset] if len(k) == 0: return False k.sort(lambda x, y: cmp(x[0], y[0])) From 2d3b13e91705a2940f8af0c3d363190bee85ea0d Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Tue, 11 Feb 2014 09:31:43 -0800 Subject: [PATCH 09/10] Add coverage command as Invoke task --- .gitignore | 1 + .travis.yml | 2 +- tasks.py | 6 +++++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 9e1febf..e149bb8 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ test.log docs/ !sites/docs _build +.coverage diff --git a/.travis.yml b/.travis.yml index df7c225..97165c4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,7 +10,7 @@ install: - pip install -r dev-requirements.txt script: # Main tests, with coverage! - - coverage run --source=paramiko test.py --verbose + - invoke coverage # Ensure documentation & invoke pipeline run OK. # Run 'docs' first since its objects.inv is referred to by 'www'. # Also force warnings to be errors since most of them tend to be actual diff --git a/tasks.py b/tasks.py index 79cb94f..f8f4017 100644 --- a/tasks.py +++ b/tasks.py @@ -26,5 +26,9 @@ www = Collection.from_module(_docs, name='www', config={ def test(ctx): ctx.run("python test.py --verbose") +@task +def coverage(ctx): + ctx.run("coverage run --source=paramiko test.py --verbose") -ns = Collection(test, docs=docs, www=www) + +ns = Collection(test, coverage, docs=docs, www=www) From c1424546219a769540fc70c54f268e284c0b2b85 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Tue, 11 Feb 2014 14:23:15 -0800 Subject: [PATCH 10/10] Start reverting explicit-release stuff in favor of release-line specifiers on bugs. --- dev-requirements.txt | 2 +- sites/www/changelog.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index e9770f4..6c242c0 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -4,4 +4,4 @@ invoke>=0.7.0 invocations>=0.4.4 sphinx>=1.1.3 alabaster>=0.3.0 -releases>=0.2.4 +releases>=0.5.0 diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst index 1a82758..af4c69e 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -12,7 +12,7 @@ Changelog * :bug:`176` Fix AttributeError bugs in known_hosts file (re)loading. Thanks to Nathan Scowcroft for the patch & Martin Blumenstingl for the initial test case. -* :release:`1.10.4 <2013-09-27>` 199, 200, 179 +* :release:`1.10.4 <2013-09-27>` * :bug:`179` Fix a missing variable causing errors when an ssh_config file has a non-default AddressFamily set. Thanks to Ed Marshall & Tomaz Muraus for catch & patch.