[merge][#109520] gc of LockableFiles should not warn or unlock
Andrew Bennetts
andrew at canonical.com
Thu Mar 20 20:18:23 GMT 2008
Martin Pool wrote:
[...]
>
> def __del__(self):
> if self.is_locked():
> - # XXX: This should show something every time, and be suitable for
> - # headless operation and embedding
> - from warnings import warn
> - warn("file group %r was not explicitly unlocked" % self)
> - self._lock.unlock()
> + mutter("file group %r was gc'd while locked" % self)
bb:approve
Separately, if that __del__ now only emits a debug warning rather than doing any
real work, it's tempting to get rid of the __del__ here entirely, so that it
can't cause the cyclic GC to fail to collect this object. Possible strategies
to do this without losing the warning are pushing the __del__ into a new object
only referenced by this object, or using weakrefs.
I'm not sure if there's been any evidence of leaks from this __del__ method, but
on prinicple if we can remove it easily then I think we should.
Here's a patch implementing the first strategy:
(I hope putting a patch in this email doesn't override your patch in
BundleBuggy, even if people don't like my patch yours should still be merged
IMO.)
=== modified file 'bzrlib/lockable_files.py'
--- bzrlib/lockable_files.py 2007-12-13 20:17:06 +0000
+++ bzrlib/lockable_files.py 2008-03-20 20:14:45 +0000
@@ -31,6 +31,24 @@
import bzrlib.urlutils as urlutils
+class _GC_Warner(object):
+ """An object that emits an warning if garbage-collected while armed."""
+
+ def __init__(self, lock_repr):
+ self.armed = False
+ self.lock_repr = lock_repr
+
+ def arm(self):
+ self.armed = True
+
+ def disarm(self):
+ self.armed = False
+
+ def __del__(self):
+ if self.armed:
+ mutter("file group %s was gc'd while locked" % (self.lock_repr,))
+
+
# XXX: The tracking here of lock counts and whether the lock is held is
# somewhat redundant with what's done in LockDir; the main difference is that
# LockableFiles permits reentrancy.
@@ -79,6 +97,7 @@
"""
self._transport = transport
self.lock_name = lock_name
+ self._gc_warner = None
self._transaction = None
self._lock_mode = None
self._lock_count = 0
@@ -88,6 +107,25 @@
file_modebits=self._file_mode,
dir_modebits=self._dir_mode)
+ def _lock_count_set(self, value):
+ """Arm or disarm the garbage-collected-while-locked warning generator
+ depending on the current value of _lock_count.
+ """
+ if value is None or value == 0:
+ if self._gc_warner is not None:
+ self._gc_warner.disarm()
+ self._gc_warner = None
+ else:
+ if self._gc_warner is None:
+ self._gc_warner = _GC_Warner(repr(self))
+ self._gc_warner.arm()
+ self._lock_count_value = value
+
+ def _lock_count_get(self):
+ return self._lock_count_value
+
+ _lock_count = property(_lock_count_get, _lock_count_set)
+
def create_lock(self):
"""Create the lock.
@@ -102,14 +140,6 @@
def __str__(self):
return 'LockableFiles(%s, %s)' % (self.lock_name, self._transport.base)
- def __del__(self):
- if self.is_locked():
- # XXX: This should show something every time, and be suitable for
- # headless operation and embedding
- from warnings import warn
- warn("file group %r was not explicitly unlocked" % self)
- self._lock.unlock()
-
def break_lock(self):
"""Break the lock of this lockable files group if it is held.
-Andrew.
More information about the bazaar
mailing list