Bazaar

Merge lp:~gz/bzr/transport_post_connect_hook into lp:bzr

  1. transport_post_connect_hook
  2. Merge into bzr.dev
Proposed by Martin Packman
Status: Superseded
Proposed branch: lp:~gz/bzr/transport_post_connect_hook
Merge into: lp:bzr
Prerequisite: lp:~spiv/bzr/hooks-refactoring
Diff against target: 280 lines (+62/-108)
6 files modified
bzrlib/hooks.py (+1/-0)
bzrlib/tests/__init__.py (+14/-9)
bzrlib/tests/test_transport.py (+23/-0)
bzrlib/tests/transport_util.py (+4/-98)
bzrlib/transport/__init__.py (+18/-1)
bzrlib/transport/remote.py (+2/-0)
To merge this branch: bzr merge lp:~gz/bzr/transport_post_connect_hook
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+38431@code.launchpad.net

This proposal has been superseded by a proposal from 2011-12-14.

Description of the change

Creates a new post_connect hook for transports, and uses it in the testing framework to ensure transports are disconnected when the test finishes so we don't leak resources. This replaces the current hack that overrides get_transport which was incomplete and causing other problems. See the mailing list thread for more:

<https://lists.ubuntu.com/archives/bazaar/2010q4/070386.html>

I'm not really sure how ready this is, but putting up an mp seems as good a way of getting feedback as bugging people on IRC.

The hook is hurled together based on lp:~parthm/bzr/173274-export-hooks if there are opinions on how it should be written differently I'd like to know.

Todo:
  News. I'm leaving this till landing actually happens to lessen pain of flux.
  Other documentation changes?
  Some tests that don't suck. Suggestions welcome.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

129 + for hook in self.hooks["post_connect"]:
130 + # GZ 2010-10-14: Should the hook be passed the new connection and
131 + # credentials too or does opaque really mean that?
132 + hook(self)

The hook already receives 'self' so it can access the connection/credentials if needed.
But they are specific to each transport class...

The tests are ok. We know the hook is heavily exercised or we get leaks anyway.

If you really really want to add tests you can check what happens when several transports share a connection, but even that sounds overkill.

Write the NEWS entry, we'll see what is available when your patch lands. Don't forget the hooks-help.txt file.

I'll ping people on the pre-requisites.

review: Approve
Revision history for this message
Martin Packman (gz) wrote :

So, very nearly this exact hook exists in bzrlib.tests.transport_util but in a hackier tests only fashion. Basically that whole file can now go, but Vincent pointed out it has some sftp-or-ftp logic depending on if paramiko is installed that may as well stay put for the moment.

Revision history for this message
Martin Packman (gz) wrote :

Just having a hook turned out to be insufficient, and results in worse hangs than the old get_transport hack as can be seen from the nearly four hour babune runtime last night:

<http://babune.ladeuil.net:24842/job/selftest-subset-windows/11/>

The problem is that RemoteTransport classes don't use _set_connection and generally behave rather differently. Throwing in a hook point in __init__ when a new medium is built gets us to a leak-free 40 minute runtime:

<http://babune.ladeuil.net:24842/job/selftest-subset-windows/13/>

However the semantics are not quite correct there. Even if we don't have to worry about remote reconnections (do we?) the post_connect hook is happening before the real connection, and potentially twice in the RemoteHTTPTransport case. Combined with the existing confusion over how many times disconnect gets called, it suggests this hook probably isn't sane enough to be generally useful. We do want the leak problem fixed asap though, so if anyone has any clever ideas...

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (4.0 KiB)

>>>>> Martin [gz] <email address hidden> writes:

    > Just having a hook turned out to be insufficient, and results in
    > worse hangs than the old get_transport hack as can be seen from
    > the nearly four hour babune runtime last night:
    > <http://babune.ladeuil.net:24842/job/selftest-subset-windows/11/>

    > The problem is that RemoteTransport classes don't use
    > _set_connection and generally behave rather differently. Throwing
    > in a hook point in __init__ when a new medium is built gets us to
    > a leak-free 40 minute runtime:

    > <http://babune.ladeuil.net:24842/job/selftest-subset-windows/13/>

Ok, so this validates the approach of calling disconnect for all
transports that have been able to connect to their server and ensures
all code paths are covered. As such, I'm tempted to accept the patch as
is waiting for a better solution in a followon.

From a design point of view, I think this outlines the divergence
between the smart transport and the others and came from the time where
we implemented connection sharing.

The smart transport has a medium object that implements
_ensure_connection() and disconnect() whereas the other transports use a
_SharedConnection object which is used a data container by the
transport, but the transport object implements connect_xxx() and
disconnect() while calling _set_connection() when the connection is
established (including reconnections).

So while the post_connect hook can be implemented for the transports
objects, it can't be properly implemented for smart transports where the
transport object is not available at the medium level when the
connection occur (in theory we could pass it done but I'm worried about
ref cycles there).

This hints as connection hook instead of a transport hook but this
doesn't play well with the transport objects who consider the connection
as an opaque attribute so far.

    > However the semantics are not quite correct there. Even if we
    > don't have to worry about remote reconnections (do we?) the
    > post_connect hook is happening before the real connection, and
    > potentially twice in the RemoteHTTPTransport case. Combined with
    > the existing confusion over how many times disconnect gets called,

I think the confusion comes the fact that disconnect() should be
implemented at the transport level since it is defined as closing the
connection even if there are other transports sharing this connection
(as opposed to closing the connection when the *last* transport using
the connection requires it).

In the test suite, all transports sharing a connection calls
disconnect() to ensure we don't get leaks but that's the expected
behaviour.

    > it suggests this hook probably isn't sane enough to be generally
    > useful.

Indeed, it lies for the smart transports as it's called *before* the
connection occurs.

    > We do want the leak problem fixed asap though, so if anyone has
    > any clever ideas...

One alternative would be to add a 'created' hook for transports which,
for the tests, will also call disconnect(). From your hack_transport
branch we know that calling disconnect() for all created transports is
enough to fix the leaks ev...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/hooks.py'
--- bzrlib/hooks.py 2010-09-28 07:34:29 +0000
+++ bzrlib/hooks.py 2010-10-15 14:01:50 +0000
@@ -80,6 +80,7 @@
80 ('bzrlib.smart.client', '_SmartClient.hooks', 'SmartClientHooks'),80 ('bzrlib.smart.client', '_SmartClient.hooks', 'SmartClientHooks'),
81 ('bzrlib.smart.server', 'SmartTCPServer.hooks', 'SmartServerHooks'),81 ('bzrlib.smart.server', 'SmartTCPServer.hooks', 'SmartServerHooks'),
82 ('bzrlib.status', 'hooks', 'StatusHooks'),82 ('bzrlib.status', 'hooks', 'StatusHooks'),
83 ('bzrlib.transport', 'Transport.hooks', 'TransportHooks'),
83 ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks',84 ('bzrlib.version_info_formats.format_rio', 'RioVersionInfoBuilder.hooks',
84 'RioVersionInfoBuilderHooks'),85 'RioVersionInfoBuilderHooks'),
85 ('bzrlib.merge_directive', 'BaseMergeDirective.hooks',86 ('bzrlib.merge_directive', 'BaseMergeDirective.hooks',
8687
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2010-10-15 11:20:45 +0000
+++ bzrlib/tests/__init__.py 2010-10-15 14:01:50 +0000
@@ -2444,15 +2444,20 @@
24442444
2445 def setUp(self):2445 def setUp(self):
2446 super(TestCaseWithMemoryTransport, self).setUp()2446 super(TestCaseWithMemoryTransport, self).setUp()
2447 # Ensure that ConnectedTransport doesn't leak sockets2447
2448 def get_transport_with_cleanup(*args, **kwargs):2448 def _add_disconnect_cleanup(transport):
2449 t = orig_get_transport(*args, **kwargs)2449 """Schedule disconnection of given transport at test cleanup
2450 if isinstance(t, _mod_transport.ConnectedTransport):2450
2451 self.addCleanup(t.disconnect)2451 This needs to happen for all connected transports or leaks occur.
2452 return t2452
24532453 Note reconnections may mean we call disconnect multiple times per
2454 orig_get_transport = self.overrideAttr(_mod_transport, 'get_transport',2454 transport which is suboptimal but seems harmless.
2455 get_transport_with_cleanup)2455 """
2456 self.addCleanup(transport.disconnect)
2457
2458 _mod_transport.Transport.hooks.install_named_hook('post_connect',
2459 _add_disconnect_cleanup, None)
2460
2456 self._make_test_root()2461 self._make_test_root()
2457 self.addCleanup(os.chdir, os.getcwdu())2462 self.addCleanup(os.chdir, os.getcwdu())
2458 self.makeAndChdirToTestDir()2463 self.makeAndChdirToTestDir()
24592464
=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py 2010-10-08 07:17:16 +0000
+++ bzrlib/tests/test_transport.py 2010-10-15 14:01:50 +0000
@@ -465,6 +465,29 @@
465 server.stop_server()465 server.stop_server()
466466
467467
468class TestHooks(tests.TestCase):
469 """Basic tests for transport hooks"""
470
471 def _get_connected_transport(self):
472 return transport.ConnectedTransport("bogus:nowhere")
473
474 def test_transporthooks_initialisation(self):
475 """Check all expected transport hook points are set up"""
476 hookpoint = transport.TransportHooks()
477 self.assertTrue("post_connect" in hookpoint,
478 "post_connect not in %s" % (hookpoint,))
479
480 def test_post_connect(self):
481 """Ensure the post_connect hook is called when _set_transport is"""
482 calls = []
483 transport.Transport.hooks.install_named_hook("post_connect",
484 calls.append, None)
485 t = self._get_connected_transport()
486 self.assertLength(0, calls)
487 t._set_connection("connection", "auth")
488 self.assertEqual(calls, [t])
489
490
468class PathFilteringDecoratorTransportTest(tests.TestCase):491class PathFilteringDecoratorTransportTest(tests.TestCase):
469 """Pathfilter decoration specific tests."""492 """Pathfilter decoration specific tests."""
470493
471494
=== modified file 'bzrlib/tests/transport_util.py'
--- bzrlib/tests/transport_util.py 2010-02-23 07:43:11 +0000
+++ bzrlib/tests/transport_util.py 2010-10-15 14:01:50 +0000
@@ -14,128 +14,34 @@
14# along with this program; if not, write to the Free Software14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
1616
17import bzrlib.hooks
18from bzrlib.tests import features17from bzrlib.tests import features
1918
20# SFTPTransport offers better performances but relies on paramiko, if paramiko19# SFTPTransport offers better performances but relies on paramiko, if paramiko
21# is not available, we fallback to FtpTransport20# is not available, we fallback to FtpTransport
22if features.paramiko.available():21if features.paramiko.available():
23 from bzrlib.tests import test_sftp_transport22 from bzrlib.tests import test_sftp_transport
24 from bzrlib.transport import sftp23 from bzrlib.transport import sftp, Transport
25 _backing_scheme = 'sftp'24 _backing_scheme = 'sftp'
26 _backing_transport_class = sftp.SFTPTransport25 _backing_transport_class = sftp.SFTPTransport
27 _backing_test_class = test_sftp_transport.TestCaseWithSFTPServer26 _backing_test_class = test_sftp_transport.TestCaseWithSFTPServer
28else:27else:
29 from bzrlib.transport import ftp28 from bzrlib.transport import ftp, Transport
30 from bzrlib.tests import test_ftp_transport29 from bzrlib.tests import test_ftp_transport
31 _backing_scheme = 'ftp'30 _backing_scheme = 'ftp'
32 _backing_transport_class = ftp.FtpTransport31 _backing_transport_class = ftp.FtpTransport
33 _backing_test_class = test_ftp_transport.TestCaseWithFTPServer32 _backing_test_class = test_ftp_transport.TestCaseWithFTPServer
3433
35from bzrlib.transport import (
36 ConnectedTransport,
37 get_transport,
38 register_transport,
39 register_urlparse_netloc_protocol,
40 unregister_transport,
41 _unregister_urlparse_netloc_protocol,
42 )
43
44
45
46class TransportHooks(bzrlib.hooks.Hooks):
47 """Dict-mapping hook name to a list of callables for transport hooks"""
48
49 def __init__(self):
50 super(TransportHooks, self).__init__()
51 # Invoked when the transport has just created a new connection.
52 # The api signature is (transport, connection, credentials)
53 self['_set_connection'] = []
54
55_hooked_scheme = 'hooked'
56
57def _change_scheme_in(url, actual, desired):
58 if not url.startswith(actual + '://'):
59 raise AssertionError('url "%r" does not start with "%r]"'
60 % (url, actual))
61 return desired + url[len(actual):]
62
63
64class InstrumentedTransport(_backing_transport_class):
65 """Instrumented transport class to test commands behavior"""
66
67 hooks = TransportHooks()
68
69 def __init__(self, base, _from_transport=None):
70 if not base.startswith(_hooked_scheme + '://'):
71 raise ValueError(base)
72 # We need to trick the backing transport class about the scheme used
73 # We'll do the reverse when we need to talk to the backing server
74 fake_base = _change_scheme_in(base, _hooked_scheme, _backing_scheme)
75 super(InstrumentedTransport, self).__init__(
76 fake_base, _from_transport=_from_transport)
77 # The following is needed to minimize the effects of our trick above
78 # while retaining the best compatibility.
79 self._scheme = _hooked_scheme
80 base = self._unsplit_url(self._scheme,
81 self._user, self._password,
82 self._host, self._port,
83 self._path)
84 super(ConnectedTransport, self).__init__(base)
85
86
87class ConnectionHookedTransport(InstrumentedTransport):
88 """Transport instrumented to inspect connections"""
89
90 def _set_connection(self, connection, credentials):
91 """Called when a new connection is created """
92 super(ConnectionHookedTransport, self)._set_connection(connection,
93 credentials)
94 for hook in self.hooks['_set_connection']:
95 hook(self, connection, credentials)
96
9734
98class TestCaseWithConnectionHookedTransport(_backing_test_class):35class TestCaseWithConnectionHookedTransport(_backing_test_class):
9936
100 def setUp(self):37 def setUp(self):
101 register_urlparse_netloc_protocol(_hooked_scheme)
102 register_transport(_hooked_scheme, ConnectionHookedTransport)
103 self.addCleanup(unregister_transport, _hooked_scheme,
104 ConnectionHookedTransport)
105 self.addCleanup(_unregister_urlparse_netloc_protocol, _hooked_scheme)
106 super(TestCaseWithConnectionHookedTransport, self).setUp()38 super(TestCaseWithConnectionHookedTransport, self).setUp()
107 self.reset_connections()39 self.reset_connections()
108 # Add the 'hooked' url to the permitted url list.
109 # XXX: See TestCase.start_server. This whole module shouldn't need to
110 # exist - a bug has been filed on that. once its cleanedup/removed, the
111 # standard test support code will work and permit the server url
112 # correctly.
113 url = self.get_url()
114 t = get_transport(url)
115 if t.base.endswith('work/'):
116 t = t.clone('../..')
117 self.permit_url(t.base)
118
119 def get_url(self, relpath=None):
120 super_self = super(TestCaseWithConnectionHookedTransport, self)
121 url = super_self.get_url(relpath)
122 # Replace the backing scheme by our own (see
123 # InstrumentedTransport.__init__)
124 url = _change_scheme_in(url, _backing_scheme, _hooked_scheme)
125 return url
12640
127 def start_logging_connections(self):41 def start_logging_connections(self):
128 self.overrideAttr(InstrumentedTransport, 'hooks', TransportHooks())42 Transport.hooks.install_named_hook('post_connect',
129 # We preserved the hooks class attribute. Now we install our hook.43 self.connections.append, None)
130 ConnectionHookedTransport.hooks.install_named_hook(
131 '_set_connection', self._collect_connection, None)
13244
133 def reset_connections(self):45 def reset_connections(self):
134 self.connections = []46 self.connections = []
13547
136 def _collect_connection(self, transport, connection, credentials):
137 # Note: uncomment the following line and use 'bt' under pdb, that will
138 # identify all the connections made including the extraneous ones.
139 # import pdb; pdb.set_trace()
140 self.connections.append(connection)
141
14248
=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py 2010-06-18 15:38:20 +0000
+++ bzrlib/transport/__init__.py 2010-10-15 14:01:50 +0000
@@ -52,7 +52,10 @@
52from bzrlib.trace import (52from bzrlib.trace import (
53 mutter,53 mutter,
54 )54 )
55from bzrlib import registry55from bzrlib import (
56 hooks,
57 registry,
58 )
5659
5760
58# a dictionary of open file streams. Keys are absolute paths, values are61# a dictionary of open file streams. Keys are absolute paths, values are
@@ -267,6 +270,16 @@
267 self.transport.append_bytes(self.relpath, bytes)270 self.transport.append_bytes(self.relpath, bytes)
268271
269272
273class TransportHooks(hooks.Hooks):
274 """Mapping of hook names to registered callbacks for transport hooks"""
275 def __init__(self):
276 super(TransportHooks, self).__init__()
277 self.create_hook(hooks.HookPoint("post_connect",
278 "Called after a new connection is established or a reconnect "
279 "occurs. The connected transport instance is the sole argument "
280 "passed.", (2, 3), None))
281
282
270class Transport(object):283class Transport(object):
271 """This class encapsulates methods for retrieving or putting a file284 """This class encapsulates methods for retrieving or putting a file
272 from/to a storage location.285 from/to a storage location.
@@ -291,6 +304,8 @@
291 # where the biggest benefit between combining reads and304 # where the biggest benefit between combining reads and
292 # and seeking is. Consider a runtime auto-tune.305 # and seeking is. Consider a runtime auto-tune.
293 _bytes_to_read_before_seek = 0306 _bytes_to_read_before_seek = 0
307
308 hooks = TransportHooks()
294309
295 def __init__(self, base):310 def __init__(self, base):
296 super(Transport, self).__init__()311 super(Transport, self).__init__()
@@ -1492,6 +1507,8 @@
1492 """1507 """
1493 self._shared_connection.connection = connection1508 self._shared_connection.connection = connection
1494 self._shared_connection.credentials = credentials1509 self._shared_connection.credentials = credentials
1510 for hook in self.hooks["post_connect"]:
1511 hook(self)
14951512
1496 def _get_connection(self):1513 def _get_connection(self):
1497 """Returns the transport specific connection object."""1514 """Returns the transport specific connection object."""
14981515
=== modified file 'bzrlib/transport/remote.py'
--- bzrlib/transport/remote.py 2010-06-18 15:38:20 +0000
+++ bzrlib/transport/remote.py 2010-10-15 14:01:50 +0000
@@ -111,6 +111,8 @@
111 if 'hpss' in debug.debug_flags:111 if 'hpss' in debug.debug_flags:
112 trace.mutter('hpss: Built a new medium: %s',112 trace.mutter('hpss: Built a new medium: %s',
113 medium.__class__.__name__)113 medium.__class__.__name__)
114 for hook in self.hooks["post_connect"]:
115 hook(self)
114 self._shared_connection = transport._SharedConnection(medium,116 self._shared_connection = transport._SharedConnection(medium,
115 credentials,117 credentials,
116 self.base)118 self.base)