Rev 2527: Finish (almost) remote refactoring. Test suite passing. in file:///v/home/vila/src/experimental/reuse.transports/

Vincent Ladeuil v.ladeuil+lp at free.fr
Fri Jun 8 09:52:30 BST 2007


At file:///v/home/vila/src/experimental/reuse.transports/

------------------------------------------------------------
revno: 2527
revision-id: v.ladeuil+lp at free.fr-20070608085227-dkwzu18gxemnwwha
parent: v.ladeuil+lp at free.fr-20070607113627-2fudc24suivry84k
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: reuse.transports
timestamp: Fri 2007-06-08 10:52:27 +0200
message:
  Finish (almost) remote refactoring. Test suite passing.
  
  * bzrlib/transport/remote.py:
  (RemoteTransport.__init__): Refactoring of connection
  sharing. This is incomplete if we want to achieve connction
  sharing across reconnections but enough to make the test suite
  pass.
  (RemoteTransport.clone): Deleted.
  (RemoteTransport._build_medium): Default implementation (mainly
  used for tests).
  (RemoteTransport.get_smart_client,
  RemoteTransport.get_smart_medium): Give back the shared medium
  instead of the private one. This is incomplete but a step in the
  right direction.
  (RemoteTCPTransport._build_medium,
  RemoteHTTPTransport._build_medium): Create and set the connection
  if not cloning.
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/tests/blackbox/test_breakin.py test_breakin.py-20070424043903-qyy6zm4pj3h4sbp3-1
  bzrlib/transport/__init__.py   transport.py-20050711165921-4978aa7ce1285ad5
  bzrlib/transport/remote.py     ssh.py-20060608202016-c25gvf1ob7ypbus6-1
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2007-06-03 12:57:26 +0000
+++ b/NEWS	2007-06-08 08:52:27 +0000
@@ -8,12 +8,16 @@
       is specified, and are labelled "revision-id:", as per mainline
       revisions, instead of "merged:". (Kent Gibson)
 
-    * Refactoring of transport classes connected to a remote
-      server. ConnectedTransport is a new class that serves as a basis for
-      all transports needing to connet to a remote server.
-      transport.split_url have been deprecated, use the static method on the
-      object instead. URL tests have been refactored too.  
-      (Vincent Ladeuil)
+    * Refactoring of transport classes connected to a remote server.
+      ConnectedTransport is a new class that serves as a basis for all
+      transports needing to connet to a remote server.  transport.split_url
+      have been deprecated, use the static method on the object instead. URL
+      tests have been refactored too.  (Vincent Ladeuil)
+
+    * Better connection sharing for ConnectedTransport objects.
+      transport.get_transport() now accepts a 'possible_transports' parameter.
+      If a newly requested transport can share a connection with one of the
+      list, it will.
 
   IMPROVEMENTS:
   
@@ -44,6 +48,13 @@
       so that we can pass in the Transport that we already have.
       (John Arbash Meinel, Vincent Ladeuil, #111702)
 
+    * Get rid of sftp cache.
+      (Vincent Ladeuil, #43731)
+
+    * All identified multiple connections for a single bzr command have been
+      fixed. See bzrlib/tests/commands directory.
+      (Vincent Ladeuil)
+
 
 bzr 0.16  2007-05-07
   

=== modified file 'bzrlib/tests/blackbox/test_breakin.py'
--- a/bzrlib/tests/blackbox/test_breakin.py	2007-04-24 04:51:31 +0000
+++ b/bzrlib/tests/blackbox/test_breakin.py	2007-06-08 08:52:27 +0000
@@ -44,7 +44,9 @@
         proc = self.start_bzr_subprocess(self._test_process_args,
                 env_changes=dict(BZR_SIGQUIT_PDB=None))
         # wait for it to get started, and print the 'listening' line
+        print 'bzr server started in test_breakin with pid: %d' % proc.pid
         proc.stdout.readline()
+        print 'stdout read'
         # first sigquit pops into debugger
         os.kill(proc.pid, signal.SIGQUIT)
         proc.stdin.write("q\n")
@@ -56,7 +58,9 @@
         proc = self.start_bzr_subprocess(self._test_process_args,
                 env_changes=dict(BZR_SIGQUIT_PDB=None))
         # wait for it to get started, and print the 'listening' line
+        print 'bzr server started in test_breakin_harder with pid: %d' % proc.pid
         proc.stdout.readline()
+        print 'stdout read'
         # another hit gives the default behaviour of terminating it
         os.kill(proc.pid, signal.SIGQUIT)
         # wait for it to go into pdb

=== modified file 'bzrlib/transport/__init__.py'
--- a/bzrlib/transport/__init__.py	2007-06-07 11:22:03 +0000
+++ b/bzrlib/transport/__init__.py	2007-06-08 08:52:27 +0000
@@ -1230,11 +1230,10 @@
             needed to create the connection.
         """
         # We use a list as a container for the connection so that the
-        # connection will be shared even if a transport is cloned before the
-        # first effective connection (generally the first request made). It
-        # also guarantees that the connection will still be shared if a
-        # transport needs to reconnect after a temporary failure.
-
+        # connection will be shared if a transport needs to reconnect after a
+        # temporary failure. It also quarantee that the connection will still
+        # be shared even if a transport is cloned before the first effective
+        # connection (generally the first request) is made.
         self._connection[0] = (connection, credentials)
 
     def _get_connection(self):

=== modified file 'bzrlib/transport/remote.py'
--- a/bzrlib/transport/remote.py	2007-06-02 16:18:53 +0000
+++ b/bzrlib/transport/remote.py	2007-06-08 08:52:27 +0000
@@ -74,6 +74,10 @@
     # RemoteTransport is an adapter from the Transport object model to the 
     # SmartClient model, not an encoder.
 
+    # FIXME: the medium parameter should be private, only the tests requires
+    # it. It may be even clearer to define a TestRemoteTransport that handles
+    # the specific cases of providing a _client and/or a _medium, and leave
+    # RemoteTransport as an abstract class.
     def __init__(self, url, from_transport=None, medium=None, _client=None):
         """Constructor.
 
@@ -89,11 +93,32 @@
             determined from the medium.
         """
         super(RemoteTransport, self).__init__(url, from_transport)
+
         if medium is None:
-            self._build_medium(from_transport)
+            medium = self._build_medium(from_transport)
         else:
-            self._medium = medium
+            self._set_connection(medium, None)
+        self._medium = medium
         assert self._medium is not None
+        # ConnectedTransport provides the connection sharing by requiring
+        # daughter classes to always access the connection via _get_connection
+        # and guarantee that the connection wil be shared across all transports
+        # even if the server force a reconnection (i.e. the connection object
+        # should be built again).
+
+        # For RemoteTransport objects, the medium is the connection. But using
+        # _get_connection is not enough because:
+        # - self._client needs a medium to be built (and keep an internal
+        #   copy).
+        # - RemoteBzrDir RemoteRepository, RemoteRepositoryFormat RemoteBranch
+        #   keep internal copies of _SmartClient objects.
+
+        # Therefore, the needed refactoring (enhancing _SmartClient and its
+        # users so that connection are still shared even in cases or
+        # reconnections) is too much for this round -- vila20070607
+
+        # On the other hand, except for the reconnection part, the sharing will
+        # already reduce the number of connections.
         if _client is None:
             self._client = client._SmartClient(self._medium)
         else:
@@ -101,24 +126,20 @@
 
     def _build_medium(self, from_transport=None):
         """Create the medium if from_transport does not provide one.
-        MUST be defined by daughter classes. The medium is
-        analogous to the connection for ConnectedTransport: it
+
+        The medium is analogous to the connection for ConnectedTransport: it
         allows connection sharing.
         
         :param from_transport: provide the medium to reuse if not None
         """
-        raise NotImplementedError(self._build_medium)
-
-    def clone(self, relative_url):
-        """Make a new RemoteTransport related to me, sharing the same connection.
-
-        This essentially opens a handle on a different remote directory.
-        """
-        if relative_url is None:
-            return self.__class__(self.base, self, self._medium)
+        if from_transport is not None:
+            _medium = from_transport._medium
         else:
-            return self.__class__(self.abspath(relative_url), self,
-                                  self._medium)
+            import pdb; pdb.set_trace()
+            _medium = None
+            # No credentials
+            self._set_connection(_medium, None)
+        return _medium
 
     def is_readonly(self):
         """Smart server transport can do read/write file operations."""
@@ -140,10 +161,10 @@
         assert False, 'weird response %r' % (resp,)
 
     def get_smart_client(self):
-        return self._medium
+        return self._get_connection()
 
     def get_smart_medium(self):
-        return self._medium
+        return self._get_connection()
 
     def _remote_path(self, relpath):
         """Returns the Unicode version of the absolute path for relpath."""
@@ -432,7 +453,8 @@
             _medium = from_transport._medium
         else:
             _medium = medium.SmartTCPClientMedium(self._host, self._port)
-        self._medium = _medium
+            self._set_connection(_medium, None)
+        return _medium
 
 
 class RemoteSSHTransport(RemoteTransport):
@@ -448,8 +470,12 @@
             _medium = from_transport._medium
         else:
             _medium = medium.SmartSSHClientMedium(self._host, self._port,
-                                                 self._user, self._password)
-        self._medium = _medium
+                                                  self._user, self._password)
+            # ssh will prompt the user for a password if needed and if none is
+            # provided but it will not give it back, so no credentials can be
+            # stored.
+            self._set_connection(_medium, None)
+        return _medium
 
 
 class RemoteHTTPTransport(RemoteTransport):
@@ -478,7 +504,9 @@
             _medium = from_transport._medium
         else:
             _medium = self._http_transport.get_smart_medium()
-        self._medium = _medium
+            # We let http_transport take care of the credentials
+            self._set_connection(_medium, None)
+        return _medium
 
     def _remote_path(self, relpath):
         """After connecting, HTTP Transport only deals in relative URLs."""



More information about the bazaar-commits mailing list