bzr connects multiple times while pushing

[Martin Pool]

I'm not sure why this is happening, but I was using 'bzr push
bzr+ssh://' to push something today, and I found that bzr actually maed
4 connections to the remote server. (This server prints a message when you connect, and I see 4 of them).

It seems to just be a problem with bzr+ssh:// because if I use bzr push sftp:// to the same machine, it only connects one time.

Now, I realize that we have an "evil" connection cache in the sftp
implementation, but connecting 4 times to push seems a little bit bad. I
don't really want to implement a connection cache, but that is probably
the easiest way to fix this. Any thoughts?

John
=:->

[John A Meinel (jameinel)]

Looking closer, I believe this is because bzr+ssh does not pass a reference to self when doing a clone(), which means it does not save the Medium, and has to reconnect.

[Wouter van Heyst (larstiq)]

On Thu, Dec 21, 2006 at 10:31:25PM -0000, John A Meinel wrote:
> Looking closer, I believe this is because bzr+ssh does not pass a
> reference to self when doing a clone(), which means it does not save the
> Medium, and has to reconnect.

Sounds similar to
http://bundlebuggy.aaronbentley.com/request/%<email address hidden>%3E

Wouter van Heyst

[Vincent Ladeuil (vila)]

Similar in the sense that it concerns clone() too, but the analogy stops here.
In the http case the connection is still shared.

[Vincent Ladeuil (vila)]

Scratch that, I meant:

The mentioned patch is similar in the sense that it concerns clone() too.

But even without that patch, in the http case, the connection was still shared.

Only the range_hints was lost, which means that bzr, in the worst case, may have to rediscover that the server can't handle ranges but will do so with the *same* connection.

Anyway, I think John is on the right track, multiple connections means either a bug in the cloning process or a failure to call clone.

[John A Meinel (jameinel)]

see also bug 86392

I tried this today, and by hiding my ssh key, I saw it connect 3 times. So we are doing slightly better as of 0.16 (RemoteTransport *does* pass around the reference)

I think this is because we are first using "to_transport = transport.get_transport(location)"
And from there we are trying to determine if the target exists or not (using BzrDir.open_from_transport(), etc)

Which is good so far.
But then bzrdir.clone() only takes a URL, not a Transport, so the transport cannot be re-used.

That accounts for 2 connections, I'm not sure how we get the 3rd.

Actually, it is probably because RemoteSSHTransport isn't properly cloning. (For one thing its __init__ doesn't have a clone_from. Second, the default RemoteTransport.clone() returns a RemoteTransport, rather than self.__class__()

[John A Meinel (jameinel)]

In debugging this, the first time it actually opens a connection is in the
BzrDir.open_from_transport() call. Because the Transport doesn't connect until necessary.

The second time is in BzrDir.clone() which calls self._make_tail(). _make_tail() also *only* works in a URL, not a Transport, so it has to create a new connection.

The final connection is BzrDir.clone() at self._format.initialize() Which also uses only a URL.

After that initialize we have an actual BzrDir object, so it maintains a transport which gets re-used.

The fix which seems most reasonable is to:

1) Update BzrDir.clone() to take a transport
2) Fix BzrDir._make_tail() to use a transport rather than a URL
3) Use BzrDirFormat.initialize_on_transport() rather than initialize()

[Robert Collins (lifeless)]

I think this is
 importance critical
because its up to 18 seconds to establish a single ssh link!

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

[Robert Collins (lifeless)]

On Tue, 2007-05-01 at 22:08 +0000, John A Meinel wrote:

> 1) Update BzrDir.clone() to take a transport
Agreed - but add a new api - clone_to_transport please. Or an optional
existing_transport parameter that can be cloned from.
> 2) Fix BzrDir._make_tail() to use a transport rather than a URL
Agreed with the same caveat about api stability - its easy to achieve
here.
> 3) Use BzrDirFormat.initialize_on_transport() rather than initialize()

Definately.
-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

[John A Meinel (jameinel)]

Fix available in the associated branch.

[John A Meinel (jameinel)]

This has been merged into bzr.dev 2479. I don't know if it will be in 0.16 or 0.17, but it *will* be in one of them.