1 |
On Mon, May 6, 2019 at 8:15 PM Zac Medico <zmedico@g.o> wrote: |
2 |
|
3 |
> Add a reentrant lock/unlock method which is useful for things like |
4 |
> emaint binhost and eclean-pkg. The vardbapi class already provides |
5 |
> lock/unlock methods that behave the same way. |
6 |
> |
7 |
|
8 |
Curious why we are not doing more encapsulation here. |
9 |
|
10 |
I'd suggest a new class type in locks.py: |
11 |
|
12 |
class RefCountedLock(object): ... |
13 |
# This implements the Lock() / Unlock() interface, as well as perhaps |
14 |
__enter__ and __exit__. |
15 |
# It uses a lockfile at path and a reference count. |
16 |
|
17 |
Then in bintree's init, set self.lock to |
18 |
locks.refCountedLock(self.bintree._pkgindex_file) |
19 |
|
20 |
def lock(self): # on bintree |
21 |
self._lock.Lock() |
22 |
|
23 |
def unlock(self): |
24 |
self._lock.Unlock() |
25 |
|
26 |
Also curious why we are not implementing enter and exit so we can avoid |
27 |
unbalanced pairs by using context managers. |
28 |
|
29 |
e.g. in match(), we could likely write: |
30 |
|
31 |
with self.dbapi.lock(): |
32 |
# try to match |
33 |
update_pkgindex = self._populate_local() |
34 |
if update_pkgindex: |
35 |
self._pkgindex_write(update_pkgindex) |
36 |
|
37 |
If the block raises an exception, __exit__ is still called, so we can |
38 |
eliminate the finally: blocks and replace them with a careful __exit__ |
39 |
implementation? |
40 |
|
41 |
-A |
42 |
|
43 |
|
44 |
> |
45 |
> Bug: https://bugs.gentoo.org/685236 |
46 |
> Signed-off-by: Zac Medico <zmedico@g.o> |
47 |
> --- |
48 |
> lib/portage/dbapi/bintree.py | 46 ++++++++++++++----- |
49 |
> lib/portage/emaint/modules/binhost/binhost.py | 9 ++-- |
50 |
> 2 files changed, 38 insertions(+), 17 deletions(-) |
51 |
> |
52 |
> diff --git a/lib/portage/dbapi/bintree.py b/lib/portage/dbapi/bintree.py |
53 |
> index 9c2d877e7..707958858 100644 |
54 |
> --- a/lib/portage/dbapi/bintree.py |
55 |
> +++ b/lib/portage/dbapi/bintree.py |
56 |
> @@ -1,4 +1,4 @@ |
57 |
> -# Copyright 1998-2018 Gentoo Foundation |
58 |
> +# Copyright 1998-2019 Gentoo Authors |
59 |
> # Distributed under the terms of the GNU General Public License v2 |
60 |
> |
61 |
> from __future__ import unicode_literals |
62 |
> @@ -94,6 +94,8 @@ class bindbapi(fakedbapi): |
63 |
> ]) |
64 |
> self._aux_cache_slot_dict = |
65 |
> slot_dict_class(self._aux_cache_keys) |
66 |
> self._aux_cache = {} |
67 |
> + self._lock = None |
68 |
> + self._lock_count = 0 |
69 |
> |
70 |
> @property |
71 |
> def writable(self): |
72 |
> @@ -106,6 +108,34 @@ class bindbapi(fakedbapi): |
73 |
> """ |
74 |
> return os.access(first_existing(self.bintree.pkgdir), |
75 |
> os.W_OK) |
76 |
> |
77 |
> + def lock(self): |
78 |
> + """ |
79 |
> + Acquire a reentrant lock, blocking, for cooperation with |
80 |
> concurrent |
81 |
> + processes. |
82 |
> + """ |
83 |
> + if self._lock_count <= 0: |
84 |
> + if self._lock is not None: |
85 |
> + raise AssertionError("already locked") |
86 |
> + # At least the parent needs to exist for the lock |
87 |
> file. |
88 |
> + ensure_dirs(self.bintree.pkgdir) |
89 |
> + self._lock = lockfile(self.bintree._pkgindex_file, |
90 |
> wantnewlockfile=True) |
91 |
> + |
92 |
> + self._lock_count += 1 |
93 |
> + |
94 |
> + def unlock(self): |
95 |
> + """ |
96 |
> + Release a lock, decrementing the recursion level. Each |
97 |
> unlock() call |
98 |
> + must be matched with a prior lock() call, or else an |
99 |
> AssertionError |
100 |
> + will be raised if unlock() is called while not locked. |
101 |
> + """ |
102 |
> + if self._lock_count <= 1: |
103 |
> + if self._lock is None: |
104 |
> + raise AssertionError("not locked") |
105 |
> + unlockfile(self._lock) |
106 |
> + self._lock = None |
107 |
> + |
108 |
> + self._lock_count -= 1 |
109 |
> + |
110 |
> def match(self, *pargs, **kwargs): |
111 |
> if self.bintree and not self.bintree.populated: |
112 |
> self.bintree.populate() |
113 |
> @@ -545,16 +575,13 @@ class binarytree(object): |
114 |
> # that changes made by a concurrent |
115 |
> process cannot be lost. This |
116 |
> # case is avoided when possible, in order |
117 |
> to minimize lock |
118 |
> # contention. |
119 |
> - pkgindex_lock = None |
120 |
> + self.dbapi.lock() |
121 |
> try: |
122 |
> - pkgindex_lock = |
123 |
> lockfile(self._pkgindex_file, |
124 |
> - wantnewlockfile=True) |
125 |
> update_pkgindex = |
126 |
> self._populate_local() |
127 |
> if update_pkgindex: |
128 |
> |
129 |
> self._pkgindex_write(update_pkgindex) |
130 |
> finally: |
131 |
> - if pkgindex_lock: |
132 |
> - unlockfile(pkgindex_lock) |
133 |
> + self.dbapi.unlock() |
134 |
> |
135 |
> if getbinpkgs: |
136 |
> if not |
137 |
> self.settings.get("PORTAGE_BINHOST"): |
138 |
> @@ -1110,10 +1137,8 @@ class binarytree(object): |
139 |
> |
140 |
> # Reread the Packages index (in case it's been changed by |
141 |
> another |
142 |
> # process) and then updated it, all while holding a lock. |
143 |
> - pkgindex_lock = None |
144 |
> + self.dbapi.lock() |
145 |
> try: |
146 |
> - pkgindex_lock = lockfile(self._pkgindex_file, |
147 |
> - wantnewlockfile=1) |
148 |
> if filename is not None: |
149 |
> new_filename = self.getname(cpv, |
150 |
> allocate_new=True) |
151 |
> try: |
152 |
> @@ -1154,8 +1179,7 @@ class binarytree(object): |
153 |
> self._pkgindex_write(pkgindex) |
154 |
> |
155 |
> finally: |
156 |
> - if pkgindex_lock: |
157 |
> - unlockfile(pkgindex_lock) |
158 |
> + self.dbapi.unlock() |
159 |
> |
160 |
> # This is used to record BINPKGMD5 in the installed package |
161 |
> # database, for a package that has just been built. |
162 |
> diff --git a/lib/portage/emaint/modules/binhost/binhost.py |
163 |
> b/lib/portage/emaint/modules/binhost/binhost.py |
164 |
> index d3df0cbce..ceb9e87f3 100644 |
165 |
> --- a/lib/portage/emaint/modules/binhost/binhost.py |
166 |
> +++ b/lib/portage/emaint/modules/binhost/binhost.py |
167 |
> @@ -1,4 +1,4 @@ |
168 |
> -# Copyright 2005-2014 Gentoo Foundation |
169 |
> +# Copyright 2005-2019 Gentoo Authors |
170 |
> # Distributed under the terms of the GNU General Public License v2 |
171 |
> |
172 |
> import errno |
173 |
> @@ -27,7 +27,6 @@ class BinhostHandler(object): |
174 |
> eroot = portage.settings['EROOT'] |
175 |
> self._bintree = portage.db[eroot]["bintree"] |
176 |
> self._bintree.populate() |
177 |
> - self._pkgindex_file = self._bintree._pkgindex_file |
178 |
> self._pkgindex = self._bintree._load_pkgindex() |
179 |
> |
180 |
> def _need_update(self, cpv, data): |
181 |
> @@ -118,9 +117,7 @@ class BinhostHandler(object): |
182 |
> missing.append(cpv) |
183 |
> |
184 |
> if missing or stale: |
185 |
> - from portage import locks |
186 |
> - pkgindex_lock = locks.lockfile( |
187 |
> - self._pkgindex_file, wantnewlockfile=1) |
188 |
> + bintree.dbapi.lock() |
189 |
> try: |
190 |
> # Repopulate with lock held. If |
191 |
> _populate_local returns |
192 |
> # data then use that, since _load_pkgindex |
193 |
> would return |
194 |
> @@ -174,7 +171,7 @@ class BinhostHandler(object): |
195 |
> bintree._pkgindex_write(self._pkgindex) |
196 |
> |
197 |
> finally: |
198 |
> - locks.unlockfile(pkgindex_lock) |
199 |
> + bintree.dbapi.unlock() |
200 |
> |
201 |
> if onProgress: |
202 |
> if maxval == 0: |
203 |
> -- |
204 |
> 2.21.0 |
205 |
> |
206 |
> |
207 |
> |