[merge][#109520] gc of LockableFiles should not warn or unlock

[Andrew Bennetts]

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.