Rev 4121: (Michael Hudson) Remove __del__ from LockableFiles. in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 4121
revision-id: pqm at pqm.ubuntu.com-20090312021804-624908fcy28eisfn
parent: pqm at pqm.ubuntu.com-20090312012854-1alv7gohle5o1wi9
parent: michael.hudson at canonical.com-20090311214421-t0n9w8akm4k50ar6
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Thu 2009-03-12 02:18:04 +0000
message:
  (Michael Hudson) Remove __del__ from LockableFiles.
modified:
  bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
    ------------------------------------------------------------
    revno: 4041.1.5
    revision-id: michael.hudson at canonical.com-20090311214421-t0n9w8akm4k50ar6
    parent: michael.hudson at canonical.com-20090227023836-htoth0azybo7tvfe
    committer: Michael Hudson <michael.hudson at canonical.com>
    branch nick: no-LockableFiles.__del__
    timestamp: Thu 2009-03-12 10:44:21 +1300
    message:
      review comments
    modified:
      bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
    ------------------------------------------------------------
    revno: 4041.1.4
    revision-id: michael.hudson at canonical.com-20090227023836-htoth0azybo7tvfe
    parent: michael.hudson at canonical.com-20090226222808-url1udsulbge2r28
    committer: Michael Hudson <michael.hudson at canonical.com>
    branch nick: no-LockableFiles.__del__
    timestamp: Fri 2009-02-27 15:38:36 +1300
    message:
      this makes more sense
    modified:
      bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
    ------------------------------------------------------------
    revno: 4041.1.3
    revision-id: michael.hudson at canonical.com-20090226222808-url1udsulbge2r28
    parent: michael.hudson at canonical.com-20090226220744-2xrce7lcr4njs1kx
    committer: Michael Hudson <michael.hudson at canonical.com>
    branch nick: no-LockableFiles.__del__
    timestamp: Fri 2009-02-27 11:28:08 +1300
    message:
      _this_ works
    modified:
      bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
    ------------------------------------------------------------
    revno: 4041.1.2
    revision-id: michael.hudson at canonical.com-20090226220744-2xrce7lcr4njs1kx
    parent: michael.hudson at canonical.com-20090226220344-ung77qhffqe5xkvo
    committer: Michael Hudson <michael.hudson at canonical.com>
    branch nick: no-LockableFiles.__del__
    timestamp: Fri 2009-02-27 11:07:44 +1300
    message:
      ahem
    modified:
      bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
    ------------------------------------------------------------
    revno: 4041.1.1
    revision-id: michael.hudson at canonical.com-20090226220344-ung77qhffqe5xkvo
    parent: pqm at pqm.ubuntu.com-20090224214044-953waog3od1tvlfr
    committer: Michael Hudson <michael.hudson at canonical.com>
    branch nick: no-LockableFiles.__del__
    timestamp: Fri 2009-02-27 11:03:44 +1300
    message:
      this is terrible but it works
    modified:
      bzrlib/lockable_files.py       control_files.py-20051111201905-bb88546e799d669f
=== modified file 'bzrlib/lockable_files.py'
--- a/bzrlib/lockable_files.py	2009-01-17 01:30:58 +0000
+++ b/bzrlib/lockable_files.py	2009-03-11 21:44:21 +0000
@@ -43,6 +43,23 @@
 # somewhat redundant with what's done in LockDir; the main difference is that
 # LockableFiles permits reentrancy.
 
+class _LockWarner(object):
+    """Hold a counter for a lock and warn if GCed while the count is >= 1.
+
+    This is separate from LockableFiles because putting a __del__ on
+    LockableFiles can result in uncollectable cycles.
+    """
+
+    def __init__(self, repr):
+        self.lock_count = 0
+        self.repr = repr
+
+    def __del__(self):
+        if self.lock_count >= 1:
+            # There should have been a try/finally to unlock this.
+            warnings.warn("%r was gc'd while locked" % self.repr)
+
+
 class LockableFiles(object):
     """Object representing a set of related files locked within the same scope.
 
@@ -88,7 +105,7 @@
         self.lock_name = lock_name
         self._transaction = None
         self._lock_mode = None
-        self._lock_count = 0
+        self._lock_warner = _LockWarner(repr(self))
         self._find_modes()
         esc_name = self._escape(lock_name)
         self._lock = lock_class(transport, esc_name,
@@ -109,12 +126,6 @@
     def __str__(self):
         return 'LockableFiles(%s, %s)' % (self.lock_name, self._transport.base)
 
-    def __del__(self):
-        if self.is_locked():
-            # do not automatically unlock; there should have been a
-            # try/finally to unlock this.
-            warnings.warn("%r was gc'd while locked" % self)
-
     def break_lock(self):
         """Break the lock of this lockable files group if it is held.
 
@@ -253,13 +264,13 @@
             if self._lock_mode != 'w' or not self.get_transaction().writeable():
                 raise errors.ReadOnlyError(self)
             self._lock.validate_token(token)
-            self._lock_count += 1
+            self._lock_warner.lock_count += 1
             return self._token_from_lock
         else:
             token_from_lock = self._lock.lock_write(token=token)
             #traceback.print_stack()
             self._lock_mode = 'w'
-            self._lock_count = 1
+            self._lock_warner.lock_count = 1
             self._set_transaction(transactions.WriteTransaction())
             self._token_from_lock = token_from_lock
             return token_from_lock
@@ -268,12 +279,12 @@
         if self._lock_mode:
             if self._lock_mode not in ('r', 'w'):
                 raise ValueError("invalid lock mode %r" % (self._lock_mode,))
-            self._lock_count += 1
+            self._lock_warner.lock_count += 1
         else:
             self._lock.lock_read()
             #traceback.print_stack()
             self._lock_mode = 'r'
-            self._lock_count = 1
+            self._lock_warner.lock_count = 1
             self._set_transaction(transactions.ReadOnlyTransaction())
             # 5K may be excessive, but hey, its a knob.
             self.get_transaction().set_cache_size(5000)
@@ -281,19 +292,23 @@
     def unlock(self):
         if not self._lock_mode:
             raise errors.LockNotHeld(self)
-        if self._lock_count > 1:
-            self._lock_count -= 1
+        if self._lock_warner.lock_count > 1:
+            self._lock_warner.lock_count -= 1
         else:
             #traceback.print_stack()
             self._finish_transaction()
             try:
                 self._lock.unlock()
             finally:
-                self._lock_mode = self._lock_count = None
+                self._lock_mode = self._lock_warner.lock_count = None
+
+    @property
+    def _lock_count(self):
+        return self._lock_warner.lock_count
 
     def is_locked(self):
         """Return true if this LockableFiles group is locked"""
-        return self._lock_count >= 1
+        return self._lock_warner.lock_count >= 1
 
     def get_physical_lock_status(self):
         """Return physical lock status.