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