Rev 2523: Finish sftp refactoring. Test suite passing. in file:///v/home/vila/src/experimental/reuse.transports/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Wed Jun 6 15:26:11 BST 2007
At file:///v/home/vila/src/experimental/reuse.transports/
------------------------------------------------------------
revno: 2523
revision-id: v.ladeuil+lp at free.fr-20070606142608-i9ufaqewadslf1cn
parent: v.ladeuil+lp at free.fr-20070606135202-mqhxcv6z57uce434
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: reuse.transports
timestamp: Wed 2007-06-06 16:26:08 +0200
message:
Finish sftp refactoring. Test suite passing.
* bzrlib/transport/sftp.py:
(clear_connection_cache): Deprecated.
(_sftp_connect, _sftp_connect_uncached): Deleted.
(SFTPTransport.__init__): Simplified.
(SFTPTransport._create_connection): New method. Copied from
_sftp_connect_uncached
(SFTPTransport._get_sftp): New method. Ensures that the connection
is established.
(SFTPTransport.clone): Deleted.
(SFTPTransport.has, SFTPTransport.get, SFTPTransport.readv,
SFTPTransport._put,
SFTPTransport._put_non_atomic_helper._open_and_write_file,
SFTPTransport._mkdir, SFTPTransport.append_file,
SFTPTransport.rename, SFTPTransport._rename_and_overwrite,
SFTPTransport.delete, SFTPTransport.rmdir, SFTPTransport.stat):
Use _get_sftp.
* bzrlib/tests/test_transport_implementations.py:
(TransportTests.test_connection_error): Simplified now that sftp
does not connection on construction.
* bzrlib/tests/test_sftp_transport.py:
(SFTPLockTests.test_sftp_locks): Delete test_multiple_connections.
(FakeSFTPTransport): Deleted.
(SFTPNonServerTest.test_parse_url_with_home_dir,
SFTPNonServerTest.test_relpath,
SSHVendorBadConnection.test_bad_connection_paramiko): Delete the
from_transport parameter as it's not needed anymore.
(SFTPLatencyKnob.test_latency_knob_slows_transport,
SFTPLatencyKnob.test_default): Force connection by issuing a
request.
modified:
bzrlib/tests/test_permissions.py test_permissions.py-20051215004520-ccf475789c80e80c
bzrlib/tests/test_sftp_transport.py testsftp.py-20051027032739-247570325fec7e7e
bzrlib/tests/test_transport_implementations.py test_transport_implementations.py-20051227111451-f97c5c7d5c49fce7
bzrlib/transport/__init__.py transport.py-20050711165921-4978aa7ce1285ad5
bzrlib/transport/sftp.py sftp.py-20051019050329-ab48ce71b7e32dfe
-------------- next part --------------
=== modified file 'bzrlib/tests/test_permissions.py'
--- a/bzrlib/tests/test_permissions.py 2006-10-11 23:08:27 +0000
+++ b/bzrlib/tests/test_permissions.py 2007-06-06 14:26:08 +0000
@@ -282,7 +282,7 @@
t.put_bytes('b', 'txt', mode=0666)
self.assertTransportMode(t, 'b', 0666)
- t._sftp.mkdir('c', mode=0777)
+ t._get_sftp().mkdir('c', mode=0777)
self.assertTransportMode(t, 'c', 0777 &~umask)
t.mkdir('d', mode=0777)
=== modified file 'bzrlib/tests/test_sftp_transport.py'
--- a/bzrlib/tests/test_sftp_transport.py 2007-06-02 14:40:26 +0000
+++ b/bzrlib/tests/test_sftp_transport.py 2007-06-06 14:26:08 +0000
@@ -104,16 +104,6 @@
l.unlock()
l2.unlock()
- def test_multiple_connections(self):
- t = self.get_transport()
- self.assertTrue('sftpserver - new connection' in self.get_server().logs)
- self.get_server().logs = []
- # The second request should reuse the first connection
- # SingleListener only allows for a single connection,
- # So the next line fails unless the connection is reused
- t2 = self.get_transport()
- self.assertEquals(self.get_server().logs, [])
-
class SFTPTransportTestRelative(TestCaseWithSFTPServer):
"""Test the SFTP transport with homedir based relative paths."""
@@ -155,17 +145,6 @@
self.assertEqual('a', t._remote_path('a'))
-class FakeSFTPTransport (object):
- _sftp = object()
- _password = None
-
- def get_connection(self):
- return None
-
-
-avoid_sftp_connection = FakeSFTPTransport()
-
-
class SFTPNonServerTest(TestCase):
def setUp(self):
TestCase.setUp(self)
@@ -173,18 +152,15 @@
raise TestSkipped('you must have paramiko to run this test')
def test_parse_url_with_home_dir(self):
- s = SFTPTransport('sftp://ro%62ey:h%40t@example.com:2222/~/relative',
- from_transport=avoid_sftp_connection)
+ s = SFTPTransport('sftp://ro%62ey:h%40t@example.com:2222/~/relative')
self.assertEquals(s._host, 'example.com')
self.assertEquals(s._port, 2222)
self.assertEquals(s._user, 'robey')
- # FIXME: sftp should just not connect at init time !!!
- #self.assertEquals(s._password, 'h at t')
+ self.assertEquals(s._password, 'h at t')
self.assertEquals(s._path, '/~/relative/')
def test_relpath(self):
- s = SFTPTransport('sftp://user@host.com/abs/path',
- from_transport=avoid_sftp_connection)
+ s = SFTPTransport('sftp://user@host.com/abs/path')
self.assertRaises(errors.PathNotChild, s.relpath,
'sftp://user@host.com/~/rel/path/sub')
@@ -339,8 +315,8 @@
"""Test that a real connection attempt raises the right error"""
from bzrlib.transport import ssh
self.set_vendor(ssh.ParamikoVendor())
- self.assertRaises(errors.ConnectionError,
- bzrlib.transport.get_transport, self.bogus_url)
+ t = bzrlib.transport.get_transport(self.bogus_url)
+ self.assertRaises(errors.ConnectionError, t.get, 'foobar')
def test_bad_connection_ssh(self):
"""None => auto-detect vendor"""
@@ -381,6 +357,7 @@
start_time = time.time()
self.get_server().add_latency = 0.5
transport = self.get_transport()
+ transport.has('not me') # Force connection by issuing a request
with_latency_knob_time = time.time() - start_time
self.assertTrue(with_latency_knob_time > 0.4)
@@ -389,6 +366,7 @@
# it could fail, but that is quite unlikely
start_time = time.time()
transport = self.get_transport()
+ transport.has('not me') # Force connection by issuing a request
regular_time = time.time() - start_time
self.assertTrue(regular_time < 0.5)
=== modified file 'bzrlib/tests/test_transport_implementations.py'
--- a/bzrlib/tests/test_transport_implementations.py 2007-06-01 20:26:46 +0000
+++ b/bzrlib/tests/test_transport_implementations.py 2007-06-06 14:26:08 +0000
@@ -962,18 +962,8 @@
except NotImplementedError:
raise TestSkipped("Transport %s has no bogus URL support." %
self._server.__class__)
- # This should be: but SSH still connects on construction. No COOKIE!
- # self.assertRaises((ConnectionError, NoSuchFile), t.get, '.bzr/branch')
- try:
- t = get_transport(url)
- t.get('.bzr/branch')
- except (ConnectionError, NoSuchFile), e:
- pass
- except (Exception), e:
- self.fail('Wrong exception thrown (%s.%s): %s'
- % (e.__class__.__module__, e.__class__.__name__, e))
- else:
- self.fail('Did not get the expected ConnectionError or NoSuchFile.')
+ t = get_transport(url)
+ self.assertRaises((ConnectionError, NoSuchFile), t.get, '.bzr/branch')
def test_stat(self):
# TODO: Test stat, just try once, and if it throws, stop testing
=== modified file 'bzrlib/transport/__init__.py'
--- a/bzrlib/transport/__init__.py 2007-06-06 13:52:02 +0000
+++ b/bzrlib/transport/__init__.py 2007-06-06 14:26:08 +0000
@@ -110,11 +110,11 @@
2) register the protocol provider with the function
register_transport_provider( ) ( and the "lazy" variant )
- This in needed because:
+ This is needed because:
a) a single provider can support multple protcol ( like the ftp
- privider which supports both the ftp:// and the aftp:// protocols )
+ provider which supports both the ftp:// and the aftp:// protocols )
b) a single protocol can have multiple providers ( like the http://
- protocol which is supported by both the urllib and pycurl privider )
+ protocol which is supported by both the urllib and pycurl provider )
"""
def register_transport_provider(self, key, obj):
@@ -1310,6 +1310,7 @@
if m:
# This looks like a URL, but we weren't able to
# instantiate it as such raise an appropriate error
+ # FIXME: we have a 'error_str' unused and we use last_err below
raise errors.UnsupportedProtocol(base, last_err)
# This doesn't look like a protocol, consider it a local path
new_base = urlutils.local_path_to_url(base)
=== modified file 'bzrlib/transport/sftp.py'
--- a/bzrlib/transport/sftp.py 2007-06-02 14:40:26 +0000
+++ b/bzrlib/transport/sftp.py 2007-06-06 14:26:08 +0000
@@ -34,7 +34,6 @@
import time
import urllib
import urlparse
-import weakref
from bzrlib import (
errors,
@@ -48,6 +47,10 @@
ParamikoNotPresent,
)
from bzrlib.osutils import pathjoin, fancy_rename, getcwd
+from bzrlib.symbol_versioning import (
+ deprecated_function,
+ zero_seventeen,
+ )
from bzrlib.trace import mutter, warning
from bzrlib.transport import (
local,
@@ -72,25 +75,18 @@
register_urlparse_netloc_protocol('sftp')
-# This is a weakref dictionary, so that we can reuse connections
-# that are still active. Long term, it might be nice to have some
-# sort of expiration policy, such as disconnect if inactive for
-# X seconds. But that requires a lot more fanciness.
-_connected_hosts = weakref.WeakValueDictionary()
-
-
_paramiko_version = getattr(paramiko, '__version_info__', (0, 0, 0))
# don't use prefetch unless paramiko version >= 1.5.5 (there were bugs earlier)
_default_do_prefetch = (_paramiko_version >= (1, 5, 5))
+ at deprecated_function(zero_seventeen)
def clear_connection_cache():
"""Remove all hosts from the SFTP connection cache.
Primarily useful for test cases wanting to force garbage collection.
+ We don't have a global connection cache anymore.
"""
- _connected_hosts.clear()
-
class SFTPLock(object):
"""This fakes a lock in a remote location.
@@ -158,12 +154,6 @@
def __init__(self, base, from_transport=None):
assert base.startswith('sftp://')
super(SFTPTransport, self).__init__(base, from_transport)
- if from_transport is None:
- self._sftp_connect()
- else:
- # use the same ssh connection, etc
- self._sftp = from_transport._sftp
- # super saves 'self.base'
def _remote_path(self, relpath):
"""Return the path to be passed along the sftp protocol for relpath.
@@ -184,29 +174,49 @@
remote_path = ''
return remote_path
+ def _create_connection(self, credentials=None):
+ """Create a new connection with the provided credentials.
+
+ :param credentials: The credentials needed to establish the connection.
+
+ :return: The created connection and its associated credentials.
+
+ The credentials are only the password as it may have been entered
+ interactively by the user and may be different from the one provided
+ in base url at transport creation time.
+ """
+ if credentials is None:
+ password = self._password
+ else:
+ password = credentials
+
+ vendor = ssh._get_ssh_vendor()
+ connection = vendor.connect_sftp(self._user, password,
+ self._host, self._port)
+ return connection, password
+
+ def _get_sftp(self):
+ """Ensures that a connection is established"""
+ connection = self._get_connection()
+ if connection is None:
+ # First connection ever
+ connection, credentials = self._create_connection()
+ self._set_connection(connection, credentials)
+ return connection
+
+
def should_cache(self):
"""
Return True if the data pulled across should be cached locally.
"""
return True
- def clone(self, offset=None):
- """
- Return a new SFTPTransport with root at self.base + offset.
- We share the same SFTP session between such transports, because it's
- fairly expensive to set them up.
- """
- if offset is None:
- return SFTPTransport(self.base, self)
- else:
- return SFTPTransport(self.abspath(offset), self)
-
def has(self, relpath):
"""
Does the target location exist?
"""
try:
- self._sftp.stat(self._remote_path(relpath))
+ self._get_sftp().stat(self._remote_path(relpath))
return True
except IOError:
return False
@@ -219,7 +229,7 @@
"""
try:
path = self._remote_path(relpath)
- f = self._sftp.file(path, mode='rb')
+ f = self._get_sftp().file(path, mode='rb')
if self._do_prefetch and (getattr(f, 'prefetch', None) is not None):
f.prefetch()
return f
@@ -236,7 +246,7 @@
try:
path = self._remote_path(relpath)
- fp = self._sftp.file(path, mode='rb')
+ fp = self._get_sftp().file(path, mode='rb')
readv = getattr(fp, 'readv', None)
if readv:
return self._sftp_readv(fp, offsets, relpath)
@@ -385,7 +395,7 @@
# Because we set_pipelined() earlier, theoretically we might
# avoid the round trip for fout.close()
if mode is not None:
- self._sftp.chmod(tmp_abspath, mode)
+ self._get_sftp().chmod(tmp_abspath, mode)
fout.close()
closed = True
self._rename_and_overwrite(tmp_abspath, abspath)
@@ -400,7 +410,7 @@
try:
if not closed:
fout.close()
- self._sftp.remove(tmp_abspath)
+ self._get_sftp().remove(tmp_abspath)
except:
# raise the saved except
raise e
@@ -421,7 +431,7 @@
fout = None
try:
try:
- fout = self._sftp.file(abspath, mode='wb')
+ fout = self._get_sftp().file(abspath, mode='wb')
fout.set_pipelined(True)
writer(fout)
except (paramiko.SSHException, IOError), e:
@@ -432,7 +442,7 @@
# Because we set_pipelined() earlier, theoretically we might
# avoid the round trip for fout.close()
if mode is not None:
- self._sftp.chmod(abspath, mode)
+ self._get_sftp().chmod(abspath, mode)
finally:
if fout is not None:
fout.close()
@@ -502,9 +512,9 @@
else:
local_mode = mode
try:
- self._sftp.mkdir(abspath, local_mode)
+ self._get_sftp().mkdir(abspath, local_mode)
if mode is not None:
- self._sftp.chmod(abspath, mode=mode)
+ self._get_sftp().chmod(abspath, mode=mode)
except (paramiko.SSHException, IOError), e:
self._translate_io_exception(e, abspath, ': unable to mkdir',
failure_exc=FileExists)
@@ -550,9 +560,9 @@
"""
try:
path = self._remote_path(relpath)
- fout = self._sftp.file(path, 'ab')
+ fout = self._get_sftp().file(path, 'ab')
if mode is not None:
- self._sftp.chmod(path, mode)
+ self._get_sftp().chmod(path, mode)
result = fout.tell()
self._pump(f, fout)
return result
@@ -562,7 +572,7 @@
def rename(self, rel_from, rel_to):
"""Rename without special overwriting"""
try:
- self._sftp.rename(self._remote_path(rel_from),
+ self._get_sftp().rename(self._remote_path(rel_from),
self._remote_path(rel_to))
except (IOError, paramiko.SSHException), e:
self._translate_io_exception(e, rel_from,
@@ -574,11 +584,13 @@
Using the implementation provided by osutils.
"""
try:
+ sftp = self._get_sftp()
fancy_rename(abs_from, abs_to,
- rename_func=self._sftp.rename,
- unlink_func=self._sftp.remove)
+ rename_func=sftp.rename,
+ unlink_func=sftp.remove)
except (IOError, paramiko.SSHException), e:
- self._translate_io_exception(e, abs_from, ': unable to rename to %r' % (abs_to))
+ self._translate_io_exception(e, abs_from,
+ ': unable to rename to %r' % (abs_to))
def move(self, rel_from, rel_to):
"""Move the item at rel_from to the location at rel_to"""
@@ -590,7 +602,7 @@
"""Delete the item at relpath"""
path = self._remote_path(relpath)
try:
- self._sftp.remove(path)
+ self._get_sftp().remove(path)
except (IOError, paramiko.SSHException), e:
self._translate_io_exception(e, path, ': unable to delete')
@@ -608,7 +620,7 @@
# -- David Allouche 2006-08-11
path = self._remote_path(relpath)
try:
- entries = self._sftp.listdir(path)
+ entries = self._get_sftp().listdir(path)
except (IOError, paramiko.SSHException), e:
self._translate_io_exception(e, path, ': failed to list_dir')
return [urlutils.escape(entry) for entry in entries]
@@ -617,7 +629,7 @@
"""See Transport.rmdir."""
path = self._remote_path(relpath)
try:
- return self._sftp.rmdir(path)
+ return self._get_sftp().rmdir(path)
except (IOError, paramiko.SSHException), e:
self._translate_io_exception(e, path, ': failed to rmdir')
@@ -625,7 +637,7 @@
"""Return the stat information for a file."""
path = self._remote_path(relpath)
try:
- return self._sftp.stat(path)
+ return self._get_sftp().stat(path)
except (IOError, paramiko.SSHException), e:
self._translate_io_exception(e, path, ': unable to stat')
@@ -655,18 +667,6 @@
# that we have taken the lock.
return SFTPLock(relpath, self)
- # FIXME: instrument or refactor to allow testing for mutiple connections
- def _sftp_connect(self):
- """Connect to the remote sftp server.
- After this, self._sftp should have a valid connection (or
- we raise an TransportError 'could not connect').
-
- TODO: Raise a more reasonable ConnectionFailed exception
- """
- self._sftp = _sftp_connect(self._host, self._port,
- self._user,
- self._password)
-
def _sftp_open_exclusive(self, abspath, mode=None):
"""Open a remote path exclusively.
@@ -685,7 +685,7 @@
# using the 'x' flag to indicate SFTP_FLAG_EXCL.
# However, there is no way to set the permission mode at open
# time using the sftp_client.file() functionality.
- path = self._sftp._adjust_cwd(abspath)
+ path = self._get_sftp()._adjust_cwd(abspath)
# mutter('sftp abspath %s => %s', abspath, path)
attr = SFTPAttributes()
if mode is not None:
@@ -693,11 +693,11 @@
omode = (SFTP_FLAG_WRITE | SFTP_FLAG_CREATE
| SFTP_FLAG_TRUNC | SFTP_FLAG_EXCL)
try:
- t, msg = self._sftp._request(CMD_OPEN, path, omode, attr)
+ t, msg = self._get_sftp()._request(CMD_OPEN, path, omode, attr)
if t != CMD_HANDLE:
raise TransportError('Expected an SFTP handle')
handle = msg.get_string()
- return SFTPFile(self._sftp, handle, 'wb', -1)
+ return SFTPFile(self._get_sftp(), handle, 'wb', -1)
except (paramiko.SSHException, IOError), e:
self._translate_io_exception(e, abspath, ': unable to open',
failure_exc=FileExists)
@@ -1029,31 +1029,6 @@
super(SFTPSiblingAbsoluteServer, self).setUp(backing_server)
-def _sftp_connect(host, port, username, password):
- """Connect to the remote sftp server.
-
- :raises: a TransportError 'could not connect'.
-
- :returns: an paramiko.sftp_client.SFTPClient
-
- TODO: Raise a more reasonable ConnectionFailed exception
- """
- idx = (host, port, username)
- try:
- return _connected_hosts[idx]
- except KeyError:
- pass
-
- sftp = _sftp_connect_uncached(host, port, username, password)
- _connected_hosts[idx] = sftp
- return sftp
-
-def _sftp_connect_uncached(host, port, username, password):
- vendor = ssh._get_ssh_vendor()
- sftp = vendor.connect_sftp(username, password, host, port)
- return sftp
-
-
def get_test_permutations():
"""Return the permutations to be used in testing."""
return [(SFTPTransport, SFTPAbsoluteServer),
More information about the bazaar-commits
mailing list