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
=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py 2010-01-12 02:48:41 +0000
+++ bzrlib/branch.py 2010-02-20 21:41:17 +0000
@@ -46,6 +46,7 @@
46 )46 )
47""")47""")
4848
49from bzrlib.contextmanagers import LockingMixin
49from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises50from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises
50from bzrlib.hooks import HookPoint, Hooks51from bzrlib.hooks import HookPoint, Hooks
51from bzrlib.inter import InterObject52from bzrlib.inter import InterObject
@@ -75,7 +76,7 @@
75######################################################################76######################################################################
76# branch objects77# branch objects
7778
78class Branch(object):79class Branch(LockingMixin):
79 """Branch holding a history of revisions.80 """Branch holding a history of revisions.
8081
81 base82 base
8283
=== added file 'bzrlib/contextmanagers.py'
--- bzrlib/contextmanagers.py 1970-01-01 00:00:00 +0000
+++ bzrlib/contextmanagers.py 2010-02-20 21:41:17 +0000
@@ -0,0 +1,79 @@
1# Copyright (C) 2010 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17"""Provide context managers via mix-ins."""
18
19__all__ = [
20 'LockingMixin',
21 'ReadLockingMixin',
22 'WriteLockingMixin',
23 ]
24
25class ReadLockingContextManager(object):
26 """Implement to context manager protocol for read locks."""
27 def __init__(self, obj):
28 self.obj = obj
29
30 def __enter__(self):
31 self.obj.lock_read()
32
33 def __exit__(self, exc_type, exc_value, traceback):
34 self.obj.unlock()
35
36
37class WriteLockingContextManager(object):
38 """Implement to context manager protocol for write locks."""
39 def __init__(self, obj, **kwargs):
40 self.obj = obj
41 self.kwargs = kwargs
42
43 def __enter__(self):
44 self.obj.lock_write(**self.kwargs)
45
46 def __exit__(self, exc_type, exc_value, traceback):
47 self.obj.unlock()
48
49
50class ReadLockingMixin(object):
51 """Add a read locking context manager to a class.
52
53 This mixin can be added to any class that provides a 'lock_read()' and a
54 'unlock()' method for locking.
55 """
56
57 def read_locking(self):
58 """Create a read locking context manager."""
59 return ReadLockingContextManager(self)
60
61
62class WriteLockingMixin(object):
63 """Add a write locking context manager to a class.
64
65 This mixin can be added to any class that provides a 'lock_write()' and a
66 'unlock()' method for locking.
67 """
68
69 def write_locking(self, **kwargs):
70 """Create a write locking context manager.
71
72 Any keyword arguments will be passed on to the 'lock_write()' method.
73 """
74 return WriteLockingContextManager(self, **kwargs)
75
76
77class LockingMixin(ReadLockingMixin, WriteLockingMixin):
78 """Add a read and a write locking context manager to a class."""
79
080
=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py 2010-02-10 16:41:09 +0000
+++ bzrlib/mutabletree.py 2010-02-20 21:41:17 +0000
@@ -28,6 +28,7 @@
28from bzrlib import (28from bzrlib import (
29 add,29 add,
30 bzrdir,30 bzrdir,
31 contextmanagers,
31 errors,32 errors,
32 hooks,33 hooks,
33 osutils,34 osutils,
@@ -54,7 +55,7 @@
54 return tree_write_locked55 return tree_write_locked
5556
5657
57class MutableTree(tree.Tree):58class MutableTree(tree.Tree, contextmanagers.WriteLockingMixin):
58 """A MutableTree is a specialisation of Tree which is able to be mutated.59 """A MutableTree is a specialisation of Tree which is able to be mutated.
5960
60 Generally speaking these mutations are only possible within a lock_write61 Generally speaking these mutations are only possible within a lock_write
6162
=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py 2010-02-17 17:42:03 +0000
+++ bzrlib/repository.py 2010-02-20 21:41:17 +0000
@@ -25,6 +25,7 @@
25 check,25 check,
26 chk_map,26 chk_map,
27 config,27 config,
28 contextmanagers,
28 debug,29 debug,
29 errors,30 errors,
30 fetch as _mod_fetch,31 fetch as _mod_fetch,
@@ -863,7 +864,7 @@
863# Repositories864# Repositories
864865
865866
866class Repository(_RelockDebugMixin):867class Repository(_RelockDebugMixin, contextmanagers.LockingMixin):
867 """Repository holding history for one or more branches.868 """Repository holding history for one or more branches.
868869
869 The repository holds and retrieves historical information including870 The repository holds and retrieves historical information including
870871
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py 2010-02-18 02:15:48 +0000
+++ bzrlib/tests/__init__.py 2010-02-20 21:41:17 +0000
@@ -3634,6 +3634,7 @@
3634 'bzrlib.tests.test_commit_merge',3634 'bzrlib.tests.test_commit_merge',
3635 'bzrlib.tests.test_config',3635 'bzrlib.tests.test_config',
3636 'bzrlib.tests.test_conflicts',3636 'bzrlib.tests.test_conflicts',
3637 'bzrlib.tests.test_contextmanagers',
3637 'bzrlib.tests.test_counted_lock',3638 'bzrlib.tests.test_counted_lock',
3638 'bzrlib.tests.test_crash',3639 'bzrlib.tests.test_crash',
3639 'bzrlib.tests.test_decorators',3640 'bzrlib.tests.test_decorators',
36403641
=== added file 'bzrlib/tests/test_contextmanagers.py'
--- bzrlib/tests/test_contextmanagers.py 1970-01-01 00:00:00 +0000
+++ bzrlib/tests/test_contextmanagers.py 2010-02-20 21:41:17 +0000
@@ -0,0 +1,167 @@
1# Copyright (C) 2010 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17
18"""Tests for context managers
19
20This test is in the odd situation that context managers are a concept that
21was introduced in Python 2.5 to work with the 'with' statement but the bzr
22test suite still runs on Python 2.4. This test module simulates the 'with'
23statement using 'try ... finally' which is also its main use case.
24
25Whenever the test suite will drop Python 2.4 compatibility, these tests
26should be rewritten to use the actual 'with' statement.
27"""
28
29from bzrlib import contextmanagers
30from bzrlib.tests import (
31 TestCase, TestCaseWithMemoryTransport, TestCaseWithTransport)
32
33
34def with_contextmanager(contextmanager, function, *args, **kwargs):
35 """Simulate the 'with' statement for a function.
36
37 Calling this function simulates this 2.5 code:
38
39 with contextmanager:
40 function(*args, **kwargs)
41
42 Since its use is limited to this test module, not all context manager
43 features are being simulated because the bzr context managers don't use
44 them.
45 - The 'as' keyword, because __enter__ will return None anyway.
46 - Exception handling in __exit__.
47 """
48
49 contextmanager.__enter__()
50 try:
51 function(*args, **kwargs)
52 finally:
53 contextmanager.__exit__(None, None, None)
54
55
56class DummyLockableObject(contextmanagers.LockingMixin):
57 """A class with lock functions to make use of the LockingMixin."""
58
59 def __init__(self):
60 """Set some counters."""
61 self.readlocked = 0
62 self.writelocked = 0
63 self.unlocked = 0
64 self.token = None
65
66 def is_locked(self):
67 """Has the object been unlocked as often as it has been locked?"""
68 return self.readlocked + self.writelocked != self.unlocked
69
70 def lock_read(self):
71 self.readlocked += 1
72
73 def lock_write(self, token=None):
74 self.writelocked += 1
75 self.token = token
76
77 def unlock(self):
78 self.unlocked += 1
79
80
81class TestReadContextManagerMixin(object):
82 """Test for read locking context manager.
83
84 Subclasses must provide self.lockable.
85 """
86
87 def with_body_read(self):
88 self.assertTrue(self.lockable.is_locked())
89
90 def test_read_locking(self):
91 with_contextmanager(
92 self.lockable.read_locking(), self.with_body_read)
93 self.assertFalse(self.lockable.is_locked())
94
95
96class TestWriteContextManagerMixin(object):
97 """Test for write locking context manager.
98
99 Subclasses must provide self.lockable.
100 """
101
102 def with_body_write(self):
103 self.assertTrue(self.lockable.is_locked())
104
105 def test_write_locking(self):
106 with_contextmanager(
107 self.lockable.write_locking(), self.with_body_write)
108 self.assertFalse(self.lockable.is_locked())
109
110
111class TestWriteTokenContextManagerMixin(object):
112 """Test for write locking context manager, passing a token.
113
114 Subclasses must provide self.lockable.
115 """
116
117 test_token = "A token"
118
119 def with_body_write_token(self):
120 self.assertEqual(self.test_token, self.lockable.token)
121
122 def test_write_locking_with_token(self):
123 with_contextmanager(
124 self.lockable.write_locking(token=self.test_token),
125 self.with_body_write_token)
126 self.assertFalse(self.lockable.is_locked())
127
128
129class TestContextManagerOnDummy(TestCase,
130 TestReadContextManagerMixin,
131 TestWriteContextManagerMixin,
132 TestWriteTokenContextManagerMixin):
133
134 def setUp(self):
135 super(TestContextManagerOnDummy, self).setUp()
136 self.lockable = DummyLockableObject()
137
138
139class TestContextManagerOnBranch(TestCaseWithMemoryTransport,
140 TestReadContextManagerMixin,
141 TestWriteContextManagerMixin):
142
143 def setUp(self):
144 super(TestContextManagerOnBranch, self).setUp()
145 self.lockable = self.make_branch('.')
146
147
148class TestContextManagerOnTree(TestCaseWithTransport,
149 TestReadContextManagerMixin,
150 TestWriteContextManagerMixin):
151
152 def with_body_read(self):
153 self.assertTrue(self.lockable.branch.is_locked())
154
155 def setUp(self):
156 super(TestContextManagerOnTree, self).setUp()
157 self.lockable = self.make_branch_and_tree('.')
158
159
160class TestContextManagerOnRepository(TestCaseWithMemoryTransport,
161 TestReadContextManagerMixin,
162 TestWriteContextManagerMixin):
163
164 def setUp(self):
165 super(TestContextManagerOnRepository, self).setUp()
166 self.lockable = repo = self.make_repository('repo')
167
0168
=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py 2010-02-17 05:12:01 +0000
+++ bzrlib/tree.py 2010-02-20 21:41:17 +0000
@@ -23,6 +23,7 @@
23import bzrlib23import bzrlib
24from bzrlib import (24from bzrlib import (
25 conflicts as _mod_conflicts,25 conflicts as _mod_conflicts,
26 contextmanagers,
26 debug,27 debug,
27 delta,28 delta,
28 filters,29 filters,
@@ -41,7 +42,7 @@
41from bzrlib.trace import note42from bzrlib.trace import note
4243
4344
44class Tree(object):45class Tree(contextmanagers.ReadLockingMixin):
45 """Abstract file tree.46 """Abstract file tree.
4647
47 There are several subclasses:48 There are several subclasses: