Merge lp:~jameinel/bzr/2.0.4-unregister-mem-trans into lp:bzr/2.0

[John A Meinel]

[John A Meinel (jameinel)]

Martin noticed that MemoryServer doesn't unregister itself. This adds that to tearDown, and adds some tests for it.

In a quick test, I did see a bunch of MemoryServer registrations left over at the end of the test suite. For example, running all of "per_pack_repository" (300 tests) left about 84 registered memory transports. From what I can tell it doesn't actually leave the MemoryServer in memory, just some functions registered in the transport_list_registry. However, it is hygienic to clean them out.

[Andrew Bennetts (spiv)]

Looks good to me.

I wonder a little if the risk/reward tradeoff is right for lp:bzr/2.0, but it's probably fine. Definitely good to have in trunk, of course.

review: Approve

[Martin Pool (mbp)]

I would not merge this to 2.0 at least without further discussion: it doesn't specifically fix serious bugs, it is fairly large and it has some risk.

I hope John just misclicked when proposing the merge.

review: Disapprove

[John A Meinel (jameinel)]

There are a fair amount of mechanical changes (move MemoryTransport => memory.MemoryTransport), but the bulk of the change is just:
255 @@ -315,11 +318,13 @@
256 result._files = self._files
257 result._locks = self._locks
258 return result
259 - register_transport(self._scheme, memory_factory)
260 + self._memory_factory = memory_factory
261 + register_transport(self._scheme, self._memory_factory)
262
263 def tearDown(self):
264 """See bzrlib.transport.Server.tearDown."""
265 # unregister this server
266 + unregister_transport(self._scheme, self._memory_factory)
267
268 def get_url(self):
269 """See bzrlib.transport.Server.get_url."""

Which I feel is sufficiently small to warrant merging to 2.0. If Martin feels differently, I'm fine only merging this to 2.1.

[Martin Pool (mbp)]

I think the main thing here is

* it doesn't actually benefit 2.0 users
* it makes the diff bigger for people to read in the next SRU
* any code change, even mechanically, has some small risk

I tried to explain this more in https://code.edge.launchpad.net/~mbp/bzr/doc/+merge/16997

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> I think the main thing here is
>
> * it doesn't actually benefit 2.0 users
> * it makes the diff bigger for people to read in the next SRU
> * any code change, even mechanically, has some small risk
>
> I tried to explain this more in https://code.edge.launchpad.net/~mbp/bzr/doc/+merge/16997

I had forgotten about the need to have a bug for SRU purposes.

Generally, I try to target anything small to 2.0, because otherwise my
default behavior is to just do everything in 'dev' and then nothing gets
into 2.0.

I'll just land this in 2.1.

 merge: rejected

John
=:->

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

iEYEARECAAYFAktHXqUACgkQJdeBCYSNAAM7HwCeJ0kQ4ybnsxLbO7JItlnJXyv5
n8MAnRzNCbfTBRUsUowPGO4w6EwovP0v
=XA3T
-----END PGP SIGNATURE-----

review: Disapprove

Unmerged revisions

4721. By John A Meinel on 2010-01-07

Add a test case.

4720. By John A Meinel on 2010-01-07

Have MemoryTransport.tearDown unregister its transport.

Small improvement, but it does free up a bit of memory, etc when running
a big test suite. (After running all of per_pack_repository, 84 memory
server instances are still registered.)

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-10-29 04:19:13 +0000
+++ bzrlib/tests/__init__.py	2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by

=== modified file 'bzrlib/tests/test_transport.py'
--- bzrlib/tests/test_transport.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_transport.py	2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2004, 2005, 2006, 2007 Canonical Ltd
+# Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -17,12 +17,15 @@
 
 from cStringIO import StringIO
 
-import bzrlib
 from bzrlib import (
     errors,
     osutils,
+    transport as _mod_transport,
     urlutils,
     )
+from bzrlib.transport import (
+    memory,
+    )
 from bzrlib.errors import (DependencyNotPresent,
                            FileExists,
                            InvalidURLJoin,
@@ -45,7 +48,6 @@
                               Transport,
                               )
 from bzrlib.transport.chroot import ChrootServer
-from bzrlib.transport.memory import MemoryTransport
 from bzrlib.transport.local import (LocalTransport,
                                     EmulatedWin32LocalTransport)
 
@@ -159,7 +161,7 @@
 
     def test_local_abspath_non_local_transport(self):
         # the base implementation should throw
-        t = MemoryTransport()
+        t = memory.MemoryTransport()
         e = self.assertRaises(errors.NotLocalUrl, t.local_abspath, 't')
         self.assertEqual('memory:///t is not a local path.', str(e))
 
@@ -249,68 +251,83 @@
                    max_size=1*1024*1024*1024)
 
 
+class TestMemoryServer(TestCase):
+
+    def test_create_server(self):
+        server = memory.MemoryServer()
+        server.setUp()
+        url = server.get_url()
+        self.assertTrue(url in _mod_transport.transport_list_registry)
+        t = _mod_transport.get_transport(url)
+        del t
+        server.tearDown()
+        self.assertFalse(url in _mod_transport.transport_list_registry)
+        self.assertRaises(errors.UnsupportedProtocol,
+                          _mod_transport.get_transport, url)
+
+
 class TestMemoryTransport(TestCase):
 
     def test_get_transport(self):
-        MemoryTransport()
+        memory.MemoryTransport()
 
     def test_clone(self):
-        transport = MemoryTransport()
-        self.assertTrue(isinstance(transport, MemoryTransport))
+        transport = memory.MemoryTransport()
+        self.assertTrue(isinstance(transport, memory.MemoryTransport))
         self.assertEqual("memory:///", transport.clone("/").base)
 
     def test_abspath(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertEqual("memory:///relpath", transport.abspath('relpath'))
 
     def test_abspath_of_root(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertEqual("memory:///", transport.base)
         self.assertEqual("memory:///", transport.abspath('/'))
 
     def test_abspath_of_relpath_starting_at_root(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertEqual("memory:///foo", transport.abspath('/foo'))
 
     def test_append_and_get(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.append_bytes('path', 'content')
         self.assertEqual(transport.get('path').read(), 'content')
         transport.append_file('path', StringIO('content'))
         self.assertEqual(transport.get('path').read(), 'contentcontent')
 
     def test_put_and_get(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.put_file('path', StringIO('content'))
         self.assertEqual(transport.get('path').read(), 'content')
         transport.put_bytes('path', 'content')
         self.assertEqual(transport.get('path').read(), 'content')
 
     def test_append_without_dir_fails(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertRaises(NoSuchFile,
                           transport.append_bytes, 'dir/path', 'content')
 
     def test_put_without_dir_fails(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertRaises(NoSuchFile,
                           transport.put_file, 'dir/path', StringIO('content'))
 
     def test_get_missing(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertRaises(NoSuchFile, transport.get, 'foo')
 
     def test_has_missing(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertEquals(False, transport.has('foo'))
 
     def test_has_present(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.append_bytes('foo', 'content')
         self.assertEquals(True, transport.has('foo'))
 
     def test_list_dir(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.put_bytes('foo', 'content')
         transport.mkdir('dir')
         transport.put_bytes('dir/subfoo', 'content')
@@ -320,28 +337,28 @@
         self.assertEquals(['subfoo'], sorted(transport.list_dir('dir')))
 
     def test_mkdir(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.mkdir('dir')
         transport.append_bytes('dir/path', 'content')
         self.assertEqual(transport.get('dir/path').read(), 'content')
 
     def test_mkdir_missing_parent(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertRaises(NoSuchFile,
                           transport.mkdir, 'dir/dir')
 
     def test_mkdir_twice(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.mkdir('dir')
         self.assertRaises(FileExists, transport.mkdir, 'dir')
 
     def test_parameters(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         self.assertEqual(True, transport.listable())
         self.assertEqual(False, transport.is_readonly())
 
     def test_iter_files_recursive(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.mkdir('dir')
         transport.put_bytes('dir/foo', 'content')
         transport.put_bytes('dir/bar', 'content')
@@ -350,7 +367,7 @@
         self.assertEqual(set(['dir/foo', 'dir/bar', 'bar']), paths)
 
     def test_stat(self):
-        transport = MemoryTransport()
+        transport = memory.MemoryTransport()
         transport.put_bytes('foo', 'content')
         transport.put_bytes('bar', 'phowar')
         self.assertEqual(7, transport.stat('foo').st_size)
@@ -420,25 +437,25 @@
 class ChrootServerTest(TestCase):
 
     def test_construct(self):
-        backing_transport = MemoryTransport()
+        backing_transport = memory.MemoryTransport()
         server = ChrootServer(backing_transport)
         self.assertEqual(backing_transport, server.backing_transport)
 
     def test_setUp(self):
-        backing_transport = MemoryTransport()
+        backing_transport = memory.MemoryTransport()
         server = ChrootServer(backing_transport)
         server.setUp()
         self.assertTrue(server.scheme in _get_protocol_handlers().keys())
 
     def test_tearDown(self):
-        backing_transport = MemoryTransport()
+        backing_transport = memory.MemoryTransport()
         server = ChrootServer(backing_transport)
         server.setUp()
         server.tearDown()
         self.assertFalse(server.scheme in _get_protocol_handlers().keys())
 
     def test_get_url(self):
-        backing_transport = MemoryTransport()
+        backing_transport = memory.MemoryTransport()
         server = ChrootServer(backing_transport)
         server.setUp()
         self.assertEqual('chroot-%d:///' % id(server), server.get_url())

=== modified file 'bzrlib/transport/memory.py'
--- bzrlib/transport/memory.py	2009-07-20 04:26:55 +0000
+++ bzrlib/transport/memory.py	2010-01-07 18:19:12 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006 Canonical Ltd
+# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -27,6 +27,9 @@
 from cStringIO import StringIO
 import warnings
 
+from bzrlib import (
+    urlutils,
+    )
 from bzrlib.errors import (
     FileExists,
     LockError,
@@ -42,8 +45,8 @@
     register_transport,
     Server,
     Transport,
+    unregister_transport,
     )
-import bzrlib.urlutils as urlutils
 
 
 
@@ -315,11 +318,13 @@
             result._files = self._files
             result._locks = self._locks
             return result
-        register_transport(self._scheme, memory_factory)
+        self._memory_factory = memory_factory
+        register_transport(self._scheme, self._memory_factory)
 
     def tearDown(self):
         """See bzrlib.transport.Server.tearDown."""
         # unregister this server
+        unregister_transport(self._scheme, self._memory_factory)
 
     def get_url(self):
         """See bzrlib.transport.Server.get_url."""