Code review comment for lp:~bzr/bzr/transport_post_connect_hook

Revision history for this message
Vincent Ladeuil (vila) wrote :

So, I think the same objection still stands: the hook is called for smart
transports *before* the connection occurs which is misleading for a
post_connect hook wanting to precisely track the effective connections.

We can rename it to connection_set...

But in any case, we should explain in the hook documentation that in some
cases, the hook will be called for a transport that hasn't establish its
connection yet.

Not pretty.

This also means that when using RemoteHTTPTransport the hook is most
probably called twice, one for the smart transport and one for the http
transport.

Basically we want to push the hook down at the medium level with some
reference to its transport (which is a good way to introduce ref-cycles ;),
wait, didn't I mention that already ?). There may be a neat trick waiting to
be found but it escapes me right now...

Looking at the smart mediums:

- the Pipe one can arguably call the hook at init time.

- the ssh based ones want to call the hook in _ensure_connection

- the http one... should not call the hook since it's backing http transport
  will. Or should it still call the hook ? There are arguments both ways
  depending on what the hook is used for...

- SmartClientAlreadyConnectedSocketMedium doesn't need to call the hook by
  definition (and the the comment in the overriding _ensure_connection is a
  hint that we are on the right track).

May be we should just focus on the test needs for now and consider yet
another hook ?

Ignoring all cloning issues and connection issues, the original need was to
disconnect all created transports without worrying about disconnecting
unused transport not transport reused multiple times.

@Jelmer: What is your need ? A precise number of connections ?

review: Needs Information

« Back to merge proposal