Bazaar

Merge lp:~henninge/bzr/bug-522195-contextmanagers into lp:bzr

  1. bug-522195-contextmanagers
  2. Merge into bzr.dev
Proposed by Henning Eggers
Status: Work in progress
Proposed branch: lp:~henninge/bzr/bug-522195-contextmanagers
Merge into: lp:bzr
Diff against target: 351 lines (+255/-4)
7 files modified
bzrlib/branch.py (+2/-1)
bzrlib/contextmanagers.py (+79/-0)
bzrlib/mutabletree.py (+2/-1)
bzrlib/repository.py (+2/-1)
bzrlib/tests/__init__.py (+1/-0)
bzrlib/tests/test_contextmanagers.py (+167/-0)
bzrlib/tree.py (+2/-1)
To merge this branch: bzr merge lp:~henninge/bzr/bug-522195-contextmanagers
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
Review via email: mp+19788@code.launchpad.net

Commit message

Added context managers for read and write locks to Branch, Tree, MutableTree, and Repository classes.

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

= Bug 522195 =

The current bzrlib is missing the ability to easily and safely lock branches, trees, and repositories using the new (well, since Python 2.5) 'with' statement. Using this construct to acquire locks on such objects makes sure they get unlocked again, even in case of an exception or a function returning within the statement. This all happens in one line with indention, making it very consise and easy to read.

This branch adds factories for context managers to Branch, Tree, and Repository objects. The factories are called 'read_locking' and 'write_locking' respectively. I chose these names, because
 - they read nicely in context of the 'with' statement because they are nouns.
 - they cannot be easily confused with 'lock_read', as would have been the case with a name like 'read_lock' (which would have been a noun, too.)

To use the context managers, simply do something like this:

  with lockable.read_locking():
      lockable.read_data()

After the with statement, the lockable is guranteed to be unlocked again, no matter what happens in its body.

I created Mixins in bzrlib.contextmanagers that were added to Branch, Tree, MutableTree, and Repository so that all subclasses of those now support this construct. Other classes in bzrlib might be lockable but I confined myself to these most important classes for now. Other classes can be extended easily using the Mixins.

I saw that some lock_write() methods take a "token" parameter although this does not seem to be the case for any of the classes mentioned above. Still, I added the ability for the write_locking() factory to pass any keyword argument into lock_write().

== Tests ==

As described in test_contextmanagers.py, compatibility with Python 2.4 makes the test of the context managers with an actual 'with' statement impossible. This test module will have to be updated once Python 2.5 is set as the lower limit.

I hope the test module is not too confusing with all the Mixins, Dummy class and with statement simulation code coming before the actual test cases.

Test command: ./bzr selftest -v contextmanagers

Reply
lp:~henninge/bzr/bug-522195-contextmanagers updated
5062. By Henning Eggers

Completed __all__ in contextmanagers module.

Revision history for this message
Robert Collins (lifeless) wrote :

I think I'd rather see these as helper functions, not mixins.

with read_locked(branch):

with write_locked(branch):

with tree_write_locked(tree):

helper functions are better in a few ways:
 - doesn't make the interface of Branch etc bigger
 - won't require plugins to be modified (that have implementations of
   Branch/Tree/Repository) to use these things
 - won't require test-per-Lockable, rather test-per-helper
 - keeps the concerns separate

Thanks for working on this.

 review: needsfixing

review: Needs Fixing
Reply
Revision history for this message
Martin Pool (mbp) wrote :

Yes, thanks.

A note in the developer documentation, perhaps in the architectural
overview, would be also good, but is not mandatory.
--
Martin <http://launchpad.net/~mbp/>

Reply
Revision history for this message
Henning Eggers (henninge) wrote :

> Yes, thanks.

I think I am missing some part of the conversation here ... ;-)

> A note in the developer documentation, perhaps in the architectural
> overview, would be also good, but is not mandatory.

Reply
Revision history for this message
Martin Pool (mbp) wrote :

On 22 February 2010 19:50, Henning Eggers <email address hidden> wrote:
>> Yes, thanks.
>
> I think I am missing some part of the conversation here ... ;-)

I was seconding Robert's "thanks for working on this."

--
Martin <http://launchpad.net/~mbp/>

Reply
Revision history for this message
Henning Eggers (henninge) wrote :

> I was seconding Robert's "thanks for working on this."

Oh, obviously. Monday mornings slowness on my part, I guess...

You're welcome. ;)

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

Marking as Work In Progress, feel free to contact us if you need help

Reply
Revision history for this message
Henning Eggers (henninge) wrote :

Yes thanks, I will. I was too busy on my last weekends to work on this. Maybe get some time in for it next weekend. ;-)

Reply

Unmerged revisions

5062. By Henning Eggers

Completed __all__ in contextmanagers module.

5061. By Henning Eggers

Merged trunk.

5060. By Henning Eggers

More doc strings.

5059. By Henning Eggers

Added locking context manager support to Repository objects.

5058. By Henning Eggers

Added test for Repository objects.

5057. By Henning Eggers

Added read locking context manager to MutableTree objects, tests passing.

5056. By Henning Eggers

Extended test for Tree to include write locking.

5055. By Henning Eggers

Added read locking context manager to Tree objects.

5054. By Henning Eggers

Added test for read locking on Tree object.

5053. By Henning Eggers

Reverted dirstate.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/branch.py'
2--- bzrlib/branch.py 2010-01-12 02:48:41 +0000
3+++ bzrlib/branch.py 2010-02-20 21:41:17 +0000
4@@ -46,6 +46,7 @@
5 )
6 """)
7
8+from bzrlib.contextmanagers import LockingMixin
9 from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
10 from bzrlib.hooks import HookPoint, Hooks
11 from bzrlib.inter import InterObject
12@@ -75,7 +76,7 @@
13 ######################################################################
14 # branch objects
15
16-class Branch(object):
17+class Branch(LockingMixin):
18 """Branch holding a history of revisions.
19
20 base
21
22=== added file 'bzrlib/contextmanagers.py'
23--- bzrlib/contextmanagers.py 1970-01-01 00:00:00 +0000
24+++ bzrlib/contextmanagers.py 2010-02-20 21:41:17 +0000
25@@ -0,0 +1,79 @@
26+# Copyright (C) 2010 Canonical Ltd
27+#
28+# This program is free software; you can redistribute it and/or modify
29+# it under the terms of the GNU General Public License as published by
30+# the Free Software Foundation; either version 2 of the License, or
31+# (at your option) any later version.
32+#
33+# This program is distributed in the hope that it will be useful,
34+# but WITHOUT ANY WARRANTY; without even the implied warranty of
35+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
36+# GNU General Public License for more details.
37+#
38+# You should have received a copy of the GNU General Public License
39+# along with this program; if not, write to the Free Software
40+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
41+
42+"""Provide context managers via mix-ins."""
43+
44+__all__ = [
45+ 'LockingMixin',
46+ 'ReadLockingMixin',
47+ 'WriteLockingMixin',
48+ ]
49+
50+class ReadLockingContextManager(object):
51+ """Implement to context manager protocol for read locks."""
52+ def __init__(self, obj):
53+ self.obj = obj
54+
55+ def __enter__(self):
56+ self.obj.lock_read()
57+
58+ def __exit__(self, exc_type, exc_value, traceback):
59+ self.obj.unlock()
60+
61+
62+class WriteLockingContextManager(object):
63+ """Implement to context manager protocol for write locks."""
64+ def __init__(self, obj, **kwargs):
65+ self.obj = obj
66+ self.kwargs = kwargs
67+
68+ def __enter__(self):
69+ self.obj.lock_write(**self.kwargs)
70+
71+ def __exit__(self, exc_type, exc_value, traceback):
72+ self.obj.unlock()
73+
74+
75+class ReadLockingMixin(object):
76+ """Add a read locking context manager to a class.
77+
78+ This mixin can be added to any class that provides a 'lock_read()' and a
79+ 'unlock()' method for locking.
80+ """
81+
82+ def read_locking(self):
83+ """Create a read locking context manager."""
84+ return ReadLockingContextManager(self)
85+
86+
87+class WriteLockingMixin(object):
88+ """Add a write locking context manager to a class.
89+
90+ This mixin can be added to any class that provides a 'lock_write()' and a
91+ 'unlock()' method for locking.
92+ """
93+
94+ def write_locking(self, **kwargs):
95+ """Create a write locking context manager.
96+
97+ Any keyword arguments will be passed on to the 'lock_write()' method.
98+ """
99+ return WriteLockingContextManager(self, **kwargs)
100+
101+
102+class LockingMixin(ReadLockingMixin, WriteLockingMixin):
103+ """Add a read and a write locking context manager to a class."""
104+
105
106=== modified file 'bzrlib/mutabletree.py'
107--- bzrlib/mutabletree.py 2010-02-10 16:41:09 +0000
108+++ bzrlib/mutabletree.py 2010-02-20 21:41:17 +0000
109@@ -28,6 +28,7 @@
110 from bzrlib import (
111 add,
112 bzrdir,
113+ contextmanagers,
114 errors,
115 hooks,
116 osutils,
117@@ -54,7 +55,7 @@
118 return tree_write_locked
119
120
121-class MutableTree(tree.Tree):
122+class MutableTree(tree.Tree, contextmanagers.WriteLockingMixin):
123 """A MutableTree is a specialisation of Tree which is able to be mutated.
124
125 Generally speaking these mutations are only possible within a lock_write
126
127=== modified file 'bzrlib/repository.py'
128--- bzrlib/repository.py 2010-02-17 17:42:03 +0000
129+++ bzrlib/repository.py 2010-02-20 21:41:17 +0000
130@@ -25,6 +25,7 @@
131 check,
132 chk_map,
133 config,
134+ contextmanagers,
135 debug,
136 errors,
137 fetch as _mod_fetch,
138@@ -863,7 +864,7 @@
139 # Repositories
140
141
142-class Repository(_RelockDebugMixin):
143+class Repository(_RelockDebugMixin, contextmanagers.LockingMixin):
144 """Repository holding history for one or more branches.
145
146 The repository holds and retrieves historical information including
147
148=== modified file 'bzrlib/tests/__init__.py'
149--- bzrlib/tests/__init__.py 2010-02-18 02:15:48 +0000
150+++ bzrlib/tests/__init__.py 2010-02-20 21:41:17 +0000
151@@ -3634,6 +3634,7 @@
152 'bzrlib.tests.test_commit_merge',
153 'bzrlib.tests.test_config',
154 'bzrlib.tests.test_conflicts',
155+ 'bzrlib.tests.test_contextmanagers',
156 'bzrlib.tests.test_counted_lock',
157 'bzrlib.tests.test_crash',
158 'bzrlib.tests.test_decorators',
159
160=== added file 'bzrlib/tests/test_contextmanagers.py'
161--- bzrlib/tests/test_contextmanagers.py 1970-01-01 00:00:00 +0000
162+++ bzrlib/tests/test_contextmanagers.py 2010-02-20 21:41:17 +0000
163@@ -0,0 +1,167 @@
164+# Copyright (C) 2010 Canonical Ltd
165+#
166+# This program is free software; you can redistribute it and/or modify
167+# it under the terms of the GNU General Public License as published by
168+# the Free Software Foundation; either version 2 of the License, or
169+# (at your option) any later version.
170+#
171+# This program is distributed in the hope that it will be useful,
172+# but WITHOUT ANY WARRANTY; without even the implied warranty of
173+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
174+# GNU General Public License for more details.
175+#
176+# You should have received a copy of the GNU General Public License
177+# along with this program; if not, write to the Free Software
178+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
179+
180+
181+"""Tests for context managers
182+
183+This test is in the odd situation that context managers are a concept that
184+was introduced in Python 2.5 to work with the 'with' statement but the bzr
185+test suite still runs on Python 2.4. This test module simulates the 'with'
186+statement using 'try ... finally' which is also its main use case.
187+
188+Whenever the test suite will drop Python 2.4 compatibility, these tests
189+should be rewritten to use the actual 'with' statement.
190+"""
191+
192+from bzrlib import contextmanagers
193+from bzrlib.tests import (
194+ TestCase, TestCaseWithMemoryTransport, TestCaseWithTransport)
195+
196+
197+def with_contextmanager(contextmanager, function, *args, **kwargs):
198+ """Simulate the 'with' statement for a function.
199+
200+ Calling this function simulates this 2.5 code:
201+
202+ with contextmanager:
203+ function(*args, **kwargs)
204+
205+ Since its use is limited to this test module, not all context manager
206+ features are being simulated because the bzr context managers don't use
207+ them.
208+ - The 'as' keyword, because __enter__ will return None anyway.
209+ - Exception handling in __exit__.
210+ """
211+
212+ contextmanager.__enter__()
213+ try:
214+ function(*args, **kwargs)
215+ finally:
216+ contextmanager.__exit__(None, None, None)
217+
218+
219+class DummyLockableObject(contextmanagers.LockingMixin):
220+ """A class with lock functions to make use of the LockingMixin."""
221+
222+ def __init__(self):
223+ """Set some counters."""
224+ self.readlocked = 0
225+ self.writelocked = 0
226+ self.unlocked = 0
227+ self.token = None
228+
229+ def is_locked(self):
230+ """Has the object been unlocked as often as it has been locked?"""
231+ return self.readlocked + self.writelocked != self.unlocked
232+
233+ def lock_read(self):
234+ self.readlocked += 1
235+
236+ def lock_write(self, token=None):
237+ self.writelocked += 1
238+ self.token = token
239+
240+ def unlock(self):
241+ self.unlocked += 1
242+
243+
244+class TestReadContextManagerMixin(object):
245+ """Test for read locking context manager.
246+
247+ Subclasses must provide self.lockable.
248+ """
249+
250+ def with_body_read(self):
251+ self.assertTrue(self.lockable.is_locked())
252+
253+ def test_read_locking(self):
254+ with_contextmanager(
255+ self.lockable.read_locking(), self.with_body_read)
256+ self.assertFalse(self.lockable.is_locked())
257+
258+
259+class TestWriteContextManagerMixin(object):
260+ """Test for write locking context manager.
261+
262+ Subclasses must provide self.lockable.
263+ """
264+
265+ def with_body_write(self):
266+ self.assertTrue(self.lockable.is_locked())
267+
268+ def test_write_locking(self):
269+ with_contextmanager(
270+ self.lockable.write_locking(), self.with_body_write)
271+ self.assertFalse(self.lockable.is_locked())
272+
273+
274+class TestWriteTokenContextManagerMixin(object):
275+ """Test for write locking context manager, passing a token.
276+
277+ Subclasses must provide self.lockable.
278+ """
279+
280+ test_token = "A token"
281+
282+ def with_body_write_token(self):
283+ self.assertEqual(self.test_token, self.lockable.token)
284+
285+ def test_write_locking_with_token(self):
286+ with_contextmanager(
287+ self.lockable.write_locking(token=self.test_token),
288+ self.with_body_write_token)
289+ self.assertFalse(self.lockable.is_locked())
290+
291+
292+class TestContextManagerOnDummy(TestCase,
293+ TestReadContextManagerMixin,
294+ TestWriteContextManagerMixin,
295+ TestWriteTokenContextManagerMixin):
296+
297+ def setUp(self):
298+ super(TestContextManagerOnDummy, self).setUp()
299+ self.lockable = DummyLockableObject()
300+
301+
302+class TestContextManagerOnBranch(TestCaseWithMemoryTransport,
303+ TestReadContextManagerMixin,
304+ TestWriteContextManagerMixin):
305+
306+ def setUp(self):
307+ super(TestContextManagerOnBranch, self).setUp()
308+ self.lockable = self.make_branch('.')
309+
310+
311+class TestContextManagerOnTree(TestCaseWithTransport,
312+ TestReadContextManagerMixin,
313+ TestWriteContextManagerMixin):
314+
315+ def with_body_read(self):
316+ self.assertTrue(self.lockable.branch.is_locked())
317+
318+ def setUp(self):
319+ super(TestContextManagerOnTree, self).setUp()
320+ self.lockable = self.make_branch_and_tree('.')
321+
322+
323+class TestContextManagerOnRepository(TestCaseWithMemoryTransport,
324+ TestReadContextManagerMixin,
325+ TestWriteContextManagerMixin):
326+
327+ def setUp(self):
328+ super(TestContextManagerOnRepository, self).setUp()
329+ self.lockable = repo = self.make_repository('repo')
330+
331
332=== modified file 'bzrlib/tree.py'
333--- bzrlib/tree.py 2010-02-17 05:12:01 +0000
334+++ bzrlib/tree.py 2010-02-20 21:41:17 +0000
335@@ -23,6 +23,7 @@
336 import bzrlib
337 from bzrlib import (
338 conflicts as _mod_conflicts,
339+ contextmanagers,
340 debug,
341 delta,
342 filters,
343@@ -41,7 +42,7 @@
344 from bzrlib.trace import note
345
346
347-class Tree(object):
348+class Tree(contextmanagers.ReadLockingMixin):
349 """Abstract file tree.
350
351 There are several subclasses: