SSH connections should be explicitly closed, not rely on gc

[Martin Pool]

follow on from https://code.launchpad.net/~mbp/bzr/rm-del-methods/+merge/64466

At the moment network connections are opened on demand, shared between related objects through possible_transports, and then closed by gc when they're no longer needed.

Generally speaking we don't want to close things through gc; also long-lived gui or server applications may want to explicitly control connection lifetime.

One way this could be done is by having a specific connection context object; there could be a default one in the library state.

This would be especially useful for SSH which currently relies on gc to do an explicit close operation (see bug 791612).

[Vincent Ladeuil (vila)]

What's wrong with transport.disconnect() ?

[Jelmer Vernooij (jelmer)]

On 04/08/11 17:37, Vincent Ladeuil wrote:
> What's wrong with transport.disconnect() ?
It requires the API user to care about the connection timeout.

E.g. something like:

<get some graph information from remote transport>
<potentially expensive processing of revisions>
<request some revisions from remote transport>

might or might not cause the connection to the remote host to time out,
depending on the timeout set on the local machine, the timeout set on
the remote machine and the amount of processing that has to be done.

Cheers,

jelmer

[Martin Pool (mbp)]

Nothing, except that it's apparently not always used. Perhaps this bug is instead to say that ssh transports should be always explicitly closed when necessary rather than relying on gc?

One possible problem with disconnect though is that it's really an operation on the connection, which might have several transports.

[Vincent Ladeuil (vila)]

> It requires the API user to care about the connection timeout.

Right, but the same kind of constraint already applies to http transports for example and the strategy there has been to reconnect transparently on transient errors. I suspect the same kind of approach could be taken for ssh transports.

The connection being shared means that all transports using it suffer/benefit in the same way.

[Martin Pool (mbp)]

I think Jelmer may be confusing this bug with the other recently
opened one about handling timeouts; they're kind of related but not
the same.

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> > It requires the API user to care about the connection timeout.
>
> Right, but the same kind of constraint already applies to http
> transports for example and the strategy there has been to reconnect
> transparently on transient errors. I suspect the same kind of approach
> could be taken for ssh transports.

Perhaps. It will be less than transparent for some users: they will get
prompted for a passphrase. And it's much more expensive in roundtrips
to reestablish an SSH session than it is to make a new HTTP connection.

Reconnecting is probably a tolerable compromise, but it's not ideal.
Ideally we'd avoid disconnecting connections unless we are certain they
aren't wanted, or are wasting resources (e.g. we are running low on
fds).

-Andrew.

[Vincent Ladeuil (vila)]

> Perhaps. It will be less than transparent for some users: they will get
> prompted for a passphrase. And it's much more expensive in roundtrips
> to reestablish an SSH session than it is to make a new HTTP connection.

Doh. If the ssh connection is dead, it needs to be re-established, whether the user is prompted or not doesn't really matter here since the goal is to prevent the bzr command to abort instead of finishing. For people using ssh agents that's a no-brainer, for people not using them, well, they'll have to enter their passphrase anyway when re-running the command (but that may fail again for the same cause... so reconnecting at least give them a chance to complete the bzr command)

Anyway, if the issue is to *not* disconnect until the last transport sharing the connection ask to, that's a different issue and will require a different method than disconnect() to track the remaining users of the connection.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/5/2011 12:37 PM, Vincent Ladeuil wrote:
>> Perhaps. It will be less than transparent for some users: they will
>> get prompted for a passphrase. And it's much more expensive in
>> roundtrips to reestablish an SSH session than it is to make a new
>> HTTP connection.
>
> Doh. If the ssh connection is dead, it needs to be re-established,
> whether the user is prompted or not doesn't really matter here since
> the goal is to prevent the bzr command to abort instead of finishing.
> For people using ssh agents that's a no-brainer, for people not using
> them, well, they'll have to enter their passphrase anyway when
> re-running the command (but that may fail again for the same cause...
> so reconnecting at least give them a chance to complete the bzr
> command)
>
> Anyway, if the issue is to *not* disconnect until the last transport
> sharing the connection ask to, that's a different issue and will
> require a different method than disconnect() to track the remaining
> users of the connection.
>

Note that on the Launchpad list they want to make it cheaper/easier to
do nodowntime deployments of codehosting. So if we had auto-reconnect,
then they wouldn't have to wait >30min to shutdown one of the processes.
(launchpad could close connections automatically when they go quiet for
a while, or only during shutdown times, etc.)

So I think Launchpad would really like it if we could auto-reconnect ssh
connections.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk47+OsACgkQJdeBCYSNAAO3BwCgxQx1suopoz0AsjoRNTBfiHvv
IhkAn329KGGCRiWrGM4DjTaJY7zX4EsF
=Ranc
-----END PGP SIGNATURE-----