[rfc] Tracking multiple connections
Vincent Ladeuil
v.ladeuil+lp at free.fr
Mon May 7 10:26:21 BST 2007
Hi,
Following a discussion on IRC with John, I want to make the
following proposal.
We have already encountered several bugs around multiple
connections to servers for a given bzr command (bug 75721 for
push and bug 111702 for init).
Several others are also waiting:
- 31140 ftplib uses more than two connections, can exceed limit
- 43731 SFTP connection cache should be removed (fixing this one
should reveal some lack of connection sharing)
Finally some tests revealed that bzr branch can also connect
multiple times and surely some others are waiting.
The implied cost, here, is that a new connection should be
established because we fail to share the transport (and its
associated connection).
So far, we reuse a connection by using transport.clone(), which
implies we have the already connected transport available.
Bug 111702 fix (pending review) implements a basic reuse
mechanism in Transport.get_transport() by allowing a
'possible_transports' parameter to be provided.
This parameter is a list of known transports provided by the
caller.
I chose a list instead of a single transport because:
- Robert mentioned that somewhere :) (can't find the reference
sorry),
- at least bzr branch can deal with two transports pointing to
different transports.
- push and pull for bound branches are likely good candidates as
they will deal with transports that may be hidden from higher
levels.
So basically we have two ways to get a new transport:
- call Transport.get_transport(url).
- clone an already available transport with a relative url.
Both are used throughout bzrlib and one possible code path is:
t = get_transport(url)
<some calls>
newurl = t.base +/- relpath
<some calls>
t2 = get_transport(newurl)
At the point where we build 't2', 't' is not in scope anymore. So
we have lost the opportunity to share the connection.
To achieve connection sharing on all the code paths where we want
it, we can:
1 - handle a global transport cache (joke, we already want to get
rid of the sftp one: bug 43731),
2 - replace the url parameter by a transport parameter in all the
code paths,
3 - add an optional 'transport' whose base is url in all the code
paths,
4 - add an optional 'possible_transports' in all the code paths.
2 smells like a compatibility nightmare (a regression in the API
ease of use even), 3 may fail to address the uses of concurrent
multiple transports, so I'm more in favor of 4 :-)
Now get_transport will be called with possible_transports and
from that list can build a new transport that will share the
connection.
The idea is that high-level commands will build the list of
reusable transports (based on the command line parameters) and
from there the transports will be cloned preserving the
connection sharing.
For the lower levels, providing such a list allows each function
to decide if she wants to just forward it, add some transports to
it or just ignore it.
Using an optional parameter also allows callers to ignore it so
that we can deploy its use progressively.
Related bugs:
https://bugs.launchpad.net/bzr/+bug/110448
https://bugs.launchpad.net/bzr/+bug/36294
https://bugs.launchpad.net/bzr/+bug/55461
I would have liked to go further and propose an API associated
with get_transport refactoring along the lines of (pseudo
incomplete code) :
def get_transport(url, possible transports):
url = normalized_url(url)
for reusable in possible_transports
transport = reusable.reuse_connection_for(url)
if transport is not None:
break
if transport is None:
transport = find one in transport factories
def reuse_connection_for(url):
scheme, host, user, password, path = split_url(url)
transport = None
if scheme, host, user, password == self.scheme, host, user, password:
transport = self.clone_or_whatever(path)
return transport
But, looking at the code :
- LocalTransport can short-circuit reuse_connection_for, no
connection to reuse,
- all the others duplicate the url splitting and connection
attributes handling using different names.
So I think a first step is to define a new
ConnectedTransport(Transport) class that will handle the
attributes extraction and define an interface to
connect/disconnect. Then we can refactor all the transport
classes to inherit from that class.
Once this refactoring is done, it will be far easier to track the
spurious connections and to deploy this proposal.
All comments welcome,
Vincent
More information about the bazaar
mailing list