[MERGE][BUG 43731][BUG 112173][BUG 111702] Transport connection sharing

Thu Jul 19 20:11:53 BST 2007

To start with, I want to thank you for doing all of this work. You've done a
lot of good stuff here, and I'm sorry it has taken so long to get reviewed.
Some of what I've said is optional, some I think is actually important. I hope
I've been clear about each statment (at least by the tone). If not, just ask.

One small thing at the beginning. We have a bit of a scattered past, but
ultimately we are avoiding CamelCase.py filenames. So

bzrlib/tests/TransportUtil.py

should probably be called

bzrlib/tests/transport_util.py

I'm guilty of EncodingAdapter because of TestUtil, but TestUtil came in because
it was copied from some other code.

TransportHooks seems like something that could be generally useful, rather than
just in the test suite.

Though I actually imagined it more as a TransportDecorator, which would
log/report everything that is done.

I think what you actually did is a bit of extra work, since you wrote
TransportHooks, so you could write a test-suite class that uses hooks, so that
you could tell when a connection was created. A whole lot of work for that a
simple decoration function would have given you.

But you've done the work, so I'm not asking you to take it out.

Using an instrumented FTP server seems a bit odd, since you have to have
'medusa' installed to run those tests. And that is the only place we require
medusa. I'm not 100% sure if it is installed on PQM (though if it isn't, it
should be).

It would be more logical to go through an instrumented sftp server, since we
use 'paramiko' and people are much more likely to have it installed. Especially
since we have already done the work to run on the loopback without any actual
ssh handling occuring.

Again, not something you *have* to fix, just something that seemed a bit better.

I would prefer it if you gave a custom protocol, like hooked+ftp://, rather
than re-using the ftp:// scheme, and assuming that your handler will win
(because it was the last one registered). It *is* likely, I don't know if it
will be always guaranteed. But it also means that you can end up with weird
failures. Where something changes, and it causes seemingly unrelated tests to
start failing.

+++ bzrlib/tests/commands/test_branch.py	2007-07-15 10:44:12 +0000
...
+    def setUp(self):
+        super(TestBranch, self).setUp()
+        self.make_branch_and_tree('branch')
+        self.install_hooks()
+        self.addCleanup(self.reset_hooks)
+

^- I don't think you need to add reset_hooks here. It is already done in
TestCaseWithConnectedTransport.setUp().
And at a minimum, it should be done by 'install_hooks'. You have this sort of
thing repeated in all of your new test code.

=== added file 'bzrlib/tests/commands/test_cat.py'

Don't forget 2 spaces between 'import' and the first class. You have this in a
lot of your new test files.

You should be using 'self.build_tree_contents()' rather than "file('xxx',
'wb').write()"

These don't really seem like 'bzrlib/tests/commands' maybe
'bzrlib/tests/command_connections'. Though it is getting a bit long.

For ftp and sftp connection caches, do we need to deprecate them first?

You have some small typos in your NEWS (connet => connect). And I would try to
put all of your entries together, since this is being merged as a single
logical chunk. (Your sftp connection cache comment is not close to your ftp
connection cache comment. I would even lump those two into a single comment).

+++ bzrlib/builtins.py

You shouldn't do:

read_bundle = bundle.read_mergeable_from_transport

At a mininum it should be "read_mergable" or maybe just "read".

I would actually prefer it if 'read_mergable_from_transport' could take the
actual transport and filename, and do its own cloning up a parent directory,
rather than having you do all of the work to normalize and split, and then
clone, etc.

Also of note is that both 'bzr merge' and 'bzr pull' have to do the same work
with a 'mergable', so it would be better to have the code factored out into
another function, that they both can call, rather than doing all the same work
each time.

This is a pretty good sign that our api's aren't what they should be:
+            create_branch = bzrdir.BzrDir.create_branch_convenience
+            branch = create_branch(to_transport.base, format=format,
+                                   possible_transports=[to_transport])

But your changes are the best way to improve it while maintaining compatibility.

+            self.outf.write("Using last location: " + display_url + '\n')
^- It is probably better not to mix "" and '' in the same string.

- -def _merge_helper(other_revision, base_revision,
+def _merge_helper(other_revision, base_revision, possible_transports=None,
                   check_clean=True, ignore_zero=False,
                   this_dir=None, backup_files=False,
                   merge_type=None,

^- Any new parameters should come at the end of the list, not in the middle.

=== modified file 'bzrlib/bundle/__init__.py'

- -            f = do_catching_redirections(get_bundle, transport,
- -                                         redirected_transport)
+            f, transport = do_catching_redirections(get_bundle, transport,
+                                                    redirected_transport)

^- Is this an api break on a public function, or is do_catching_redirections
just a local func?

=== modified file 'bzrlib/bzrdir.py'
- -        return format.initialize(safe_unicode(base))
+        return format.initialize(base, possible_transports)

^- A parameter like possible_transports really seems like it should be passed
in as a keyword, not as a positional argument.

return format.initialize(base, possible_transports=possible_transports)

v- same here
- -            t = get_transport(safe_unicode(base))
+            t = get_transport(base, possible_transports)

This is also something that happens repeatedly, so a find/replace is probably
better than me hunting all of them down.

- -        # TODO: jam 20060426 we probably need a test in here in the
- -        #       case that the newly sprouted branch is a remote one
- -        if result_repo is None or result_repo.make_working_trees():
+        if isinstance(target_transport, LocalTransport) and (
+            result_repo is None or result_repo.make_working_trees()):
             wt = result.create_workingtree()
             wt.lock_write()
             try:

^- This doesn't seem like the time/place to be fixing this. But did you at
least add tests that 'bzr branch local sftp://remote' doesn't try to create a
working tree?

- -class PathNotChild(BzrError):
+class PathNotChild(PathError):

^- Is there a specific reason for this? It seems correct, but I didn't see an
explanation.

+        # vila--20070607 if the following are commented out the test suite
+        # still pass. Is this really still needed or was it a forgotten
+        # temporary fix ?
         if not isinstance(t, self.transport_class):
             # we did not get the correct transport class type. Override the
             # regular connection behaviour by direct construction.

Can you elaborate a bit on this?
It looks like something Robrert wrote, so I don't know what is going on myself.

...

         self.assertEquals(t.base, 'file://HOST/')

+class TestConnectedTransport(TestCase):
+    """Tests for connected to remote server transports"""
+

^- 2 spaces please

v- Nice to see this done, thank you.
- -        # This should be:  but SSH still connects on construction. No COOKIE!
- -        # self.assertRaises((ConnectionError, NoSuchFile), t.get, '.bzr/branch')
- -        try:
- -            t = bzrlib.transport.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')

+class ConnectedTransport(Transport):
+    """A transport connected to a remote server.
+
+    This class provide the basis to implement transports that need to connect
+    to a remote server.
+
+    Host and credentials are available as private attributes, cloning preserves
+    them and share the underlying, protocol specific, connection.
+    """
+
+    def __init__(self, base, from_transport=None):
+        """Constructor.
+
+        :param base: transport root URL
+
+        :param from_transport: optional transport to build from. The built
+            transport will share the connection with this transport.
+        """
+        if base[-1] != '/':
+            base += '/'

^- This should always be:

if not base.endswith('/'):
  base += '/'

+        (self._scheme,
+         self._user, self._password,
+         self._host, self._port,
+         self._path) = self._split_url(base)

^- It seems a little odd that you will split the scheme, etc from 'base' but
use the connection from 'from_transport'. At a minimum, it seems like they
should be verified to match.

+        if from_transport is not None:
+            # Copy the password as it does not appear in base and will be lost
+            # otherwise. It can appear in the _initial_split_url above if the
+            # user provided it on the command line. Otherwise, daughter classes
+            # will prompt the user for one when appropriate.
+            self._password = from_transport._password
+
+        base = self._unsplit_url(self._scheme,
+                                 self._user, self._password,
+                                 self._host, self._port,
+                                 self._path)

^- I also thought we tended to remove 'self._password' from the official URL,
so that errors/etc won't expose passwords to the terminal or to the log.

This also seems a good place to use keywords:

base = self._unsplit_url(scheme=self._scheme,
			 user=self._user,
			 ...
			)

Also, if _unsplit_url is only being called with self._* parameters, it should
probably just use them directly, rather than having them be passed in.

I do notice that _unsplit_url is ignoring the password parameter. So it would
probably be best to just not pass it in.

self._path might be better understood as self._remote_base_path.

We need a decent audit that ConnectedTransport doesn't get around the abilities
of ChrootTransportDecorator.

v- I actually think:

if (scheme == self._scheme
    and user == self._user
    and host == self._host
    and port == self._port):

is easier to read.

+        if (scheme, user, host, port) == (self._scheme,
+                                          self._user, self._host, self._port):
+            if not path.endswith('/'):
+                # This normally occurs at __init__ time, but it's easier to do
+                # it now to avoid creating two transports for the same base.
+                path += '/'
+            if self._path  == path:
+                # shortcut, it's really the same transport
+                return self

- -# TODO: The should be marked as deprecated
+# TODO: They should be marked as deprecated
+# vila-20070606: How should we do that ?
 urlescape = urlutils.escape
 urlunescape = urlutils.unescape

^- Probably:

urlescape = deprecated_function(zero_XXXX, urlutils.escape)

I was thinking we would need to create a wrapper function, but deprecated_*
creates its own wrapper function.

I think by this time, we could actually nuke them, but I suppose it is possible
they are being used (since we haven't actually caused code to issue deprecation
warnings).

+# FIXME: there are inconsistencies in the way temporary errors are
+# handled. Sometimes we reconnect, sometimes we raise an exception. Care should
+# be taken to analyze the implications for write operations (read operations
+# are safe to retry). Overall even some read operations are never retried.

^- Add vila and date. Also, this should be filed as a bug if you haven't
already done so.

+# FIXME: There are two known cases where we open a new connection during the
+# lifetime of the transport:
+# - when an error is received from the server,
+# - when the keep-alive header reach zero.
+# This should be taken into account for the connection sharing by either
+# failing without reconnecting or inform the clones that a new connection have
+# been established.

^- Same here

And we will definitely reach "keep-alive header reach zero". Because Apache
defaults to either 100 or 200 requests. And branching bzr.dev creates *thousands*.

=== modified file 'bzrlib/transport/http/_urllib.py'

- -        self._connection.fake_close()
+        self._get_connection().fake_close()

^- This doesn't seem quite right. You are potentially re-connecting just so
that you can 'fake_close'. Maybe we need a self._fake_close() that uses a bit
more direct knowledge about what is going on, rather than just following the
same _get_connection() logic.

Overall, I think you did a great job. I would be happy enough to just merge it
as is. Though there are a few things that could be made better/clearer. +0.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGn7d4JdeBCYSNAAMRArJYAJ0VNuCNa51unEC0Wxv5uB/AKcTzRACfUV9+
9WCR6PXLLLKehdMww8a7Z6g=
=heev
-----END PGP SIGNATURE-----