Merge lp:~mbp/bzr/rm-del-methods into lp:bzr

[Martin Pool]

remove most __del__ methods

Per bug 791612 and recent discussion about bad gc behavior of __del__ methods, this just removes them.

[Martin Pool (mbp)]

https://bugs.launchpad.net/launchpad/+bug/721166 is another related case, where a warning is intermittently breaking lp's test suite, but without actually telling us anything useful.

[Martin Pool (mbp)]

I'm thinking of also banning 'def __del__(' the same way we ban assert, to avoid later disappointment.

[Martin Pool (mbp)]

... but perhaps we can wait til it's an actual problem, or until we're sure there's really zero use for it.

[Martin Pool (mbp)]

also bug 391024 showing these methods generate noise without helping very much in removing leaks

[John A Meinel (jameinel)]

I'm fine with all of them but the SmartStreamMedium. That one is potentially doing real work in __del__

review: Needs Information

[Martin Pool (mbp)]

On 14 June 2011 02:58, John A Meinel <email address hidden> wrote:
> Review: Needs Information
> I'm fine with all of them but the SmartStreamMedium. That one is potentially doing real work in __del__

It is, but I think not in a way that's going to close very safely or
reliably. There is no real guarantee things will still be
sufficiently open when this fires that this can actually disconnect.

I can't find any specific bugs from this but it looks a lot like the
kind of thing that gives spurious errors.

Possibly I should audit for places where this is constructed and check
they're explicitly closed.

I might wait on this for comments from spiv.

[Andrew Bennetts (spiv)]

I'd be pretty hesitant about removing the smart medium __del__ too. I'm worried we'd see a recurrence of bug 659590 or similar if we did, because I don't think we typically do explicitly .disconnect() Transports (and thus any associated medium) very often at all.

In principle we ought to be explicitly releasing those resources, so in principle removing the __del__ as a fallback is fine, but I think reality doesn't yet match that principle.

[Martin Pool (mbp)]

I'll put back the ssh gc and file a separate bug for deterministically closing them.

[Martin Pool (mbp)]

https://bugs.launchpad.net/bzr/+bug/803187

and this has now been overwritten with a version that leaves the del method in place for the smart medium.

[Martin Pool (mbp)]

another review, please?

[Martin Pool (mbp)]

based on previous comments and offline discussion, I'm going to send this in

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/atomicfile.py'
--- bzrlib/atomicfile.py	2011-05-20 14:46:02 +0000
+++ bzrlib/atomicfile.py	2011-06-28 21:53:58 +0000
@@ -118,7 +118,3 @@
         """Discard the file unless already committed."""
         if self._fd is not None:
             self.abort()
-
-    def __del__(self):
-        if self._fd is not None:
-            warnings.warn("%r leaked" % self)

=== modified file 'bzrlib/lock.py'
--- bzrlib/lock.py	2011-03-30 11:45:54 +0000
+++ bzrlib/lock.py	2011-06-28 21:53:58 +0000
@@ -171,12 +171,6 @@
             self.f.close()
             self.f = None
 
-    def __del__(self):
-        if self.f:
-            from warnings import warn
-            warn("lock on %r not released" % self.f)
-            self.unlock()
-
     def unlock(self):
         raise NotImplementedError()
 

=== modified file 'bzrlib/lockable_files.py'
--- bzrlib/lockable_files.py	2011-04-07 10:36:24 +0000
+++ bzrlib/lockable_files.py	2011-06-28 21:53:58 +0000
@@ -33,27 +33,6 @@
     )
 
 
-# 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.
-
-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.
 
@@ -68,17 +47,14 @@
     This class is now deprecated; code should move to using the Transport
     directly for file operations and using the lock or CountedLock for
     locking.
-    
+
     :ivar _lock: The real underlying lock (e.g. a LockDir)
-    :ivar _counted_lock: A lock decorated with a semaphore, so that it 
-        can be re-entered.
+    :ivar _lock_count: If _lock_mode is true, a positive count of the number
+        of times the lock has been taken (and not yet released) *by this
+        process*, through this particular object instance.
+    :ivar _lock_mode: None, or 'r' or 'w'
     """
 
-    # _lock_mode: None, or 'r' or 'w'
-
-    # _lock_count: If _lock_mode is true, a positive count of the number of
-    # times the lock has been taken *by this process*.
-
     def __init__(self, transport, lock_name, lock_class):
         """Create a LockableFiles group
 
@@ -92,7 +68,7 @@
         self.lock_name = lock_name
         self._transaction = None
         self._lock_mode = None
-        self._lock_warner = _LockWarner(repr(self))
+        self._lock_count = 0
         self._find_modes()
         esc_name = self._escape(lock_name)
         self._lock = lock_class(transport, esc_name,
@@ -111,6 +87,7 @@
     def __repr__(self):
         return '%s(%r)' % (self.__class__.__name__,
                            self._transport)
+
     def __str__(self):
         return 'LockableFiles(%s, %s)' % (self.lock_name, self._transport.base)
 
@@ -174,19 +151,18 @@
         some other way, and need to synchronise this object's state with that
         fact.
         """
-        # TODO: Upgrade locking to support using a Transport,
-        # and potentially a remote locking protocol
         if self._lock_mode:
-            if self._lock_mode != 'w' or not self.get_transaction().writeable():
+            if (self._lock_mode != 'w'
+                or not self.get_transaction().writeable()):
                 raise errors.ReadOnlyError(self)
             self._lock.validate_token(token)
-            self._lock_warner.lock_count += 1
+            self._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_warner.lock_count = 1
+            self._lock_count = 1
             self._set_write_transaction()
             self._token_from_lock = token_from_lock
             return token_from_lock
@@ -195,12 +171,12 @@
         if self._lock_mode:
             if self._lock_mode not in ('r', 'w'):
                 raise ValueError("invalid lock mode %r" % (self._lock_mode,))
-            self._lock_warner.lock_count += 1
+            self._lock_count += 1
         else:
             self._lock.lock_read()
             #traceback.print_stack()
             self._lock_mode = 'r'
-            self._lock_warner.lock_count = 1
+            self._lock_count = 1
             self._set_read_transaction()
 
     def _set_read_transaction(self):
@@ -217,23 +193,19 @@
     def unlock(self):
         if not self._lock_mode:
             return lock.cant_unlock_not_held(self)
-        if self._lock_warner.lock_count > 1:
-            self._lock_warner.lock_count -= 1
+        if self._lock_count > 1:
+            self._lock_count -= 1
         else:
             #traceback.print_stack()
             self._finish_transaction()
             try:
                 self._lock.unlock()
             finally:
-                self._lock_mode = self._lock_warner.lock_count = None
-
-    @property
-    def _lock_count(self):
-        return self._lock_warner.lock_count
+                self._lock_mode = self._lock_count = None
 
     def is_locked(self):
         """Return true if this LockableFiles group is locked"""
-        return self._lock_warner.lock_count >= 1
+        return self._lock_count >= 1
 
     def get_physical_lock_status(self):
         """Return physical lock status.
@@ -325,4 +297,3 @@
     def validate_token(self, token):
         if token is not None:
             raise errors.TokenLockingNotSupported(self)
-

=== modified file 'bzrlib/tests/test_fifo_cache.py'
--- bzrlib/tests/test_fifo_cache.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_fifo_cache.py	2011-06-28 21:53:58 +0000
@@ -229,11 +229,11 @@
         c = fifo_cache.FIFOCache()
         c.add(1, 2, cleanup=logging_cleanup)
         del c
-        # TODO: We currently don't support calling the cleanup() funcs during
-        #       __del__. We might want to consider implementing this.
-        self.expectFailure("we don't call cleanups during __del__",
-                           self.assertEqual, [(1, 2)], log)
-        self.assertEqual([(1, 2)], log)
+        # As a matter of design, bzr does not (can not) count on anything
+        # being run from Python __del__ methods, because they may not run for
+        # a long time, and because in cPython merely having them defined
+        # interferes with garbage collection.
+        self.assertEqual([], log)
 
 
 class TestFIFOSizeCache(tests.TestCase):

=== modified file 'bzrlib/transport/memory.py'
--- bzrlib/transport/memory.py	2010-02-23 07:43:11 +0000
+++ bzrlib/transport/memory.py	2011-06-28 21:53:58 +0000
@@ -289,12 +289,6 @@
             raise LockError('File %r already locked' % (self.path,))
         self.transport._locks[self.path] = self
 
-    def __del__(self):
-        # Should this warn, or actually try to cleanup?
-        if self.transport:
-            warnings.warn("MemoryLock %r not explicitly unlocked" % (self.path,))
-            self.unlock()
-
     def unlock(self):
         del self.transport._locks[self.path]
         self.transport = None

=== modified file 'bzrlib/transport/sftp.py'
--- bzrlib/transport/sftp.py	2011-04-20 14:27:19 +0000
+++ bzrlib/transport/sftp.py	2011-06-28 21:53:58 +0000
@@ -114,12 +114,6 @@
         except FileExists:
             raise LockError('File %r already locked' % (self.path,))
 
-    def __del__(self):
-        """Should this warn, or actually try to cleanup?"""
-        if self.lock_file:
-            warning("SFTPLock %r not explicitly unlocked" % (self.path,))
-            self.unlock()
-
     def unlock(self):
         if not self.lock_file:
             return

=== modified file 'doc/developers/code-style.txt'
--- doc/developers/code-style.txt	2010-12-14 23:39:41 +0000
+++ doc/developers/code-style.txt	2011-06-28 21:53:58 +0000
@@ -189,15 +189,21 @@
    why in a comment.
 
 1. Never rely on a ``__del__`` method running.  If there is code that
-   must run, do it from a ``finally`` block instead.
+   must run, instead have a ``finally`` block or an ``addCleanup`` call an
+   explicit ``close`` method.
 
 2. Never ``import`` from inside a ``__del__`` method, or you may crash the
    interpreter!!
 
-3. In some places we raise a warning from the destructor if the object
-   has not been cleaned up or closed.  This is considered OK: the warning
-   may not catch every case but it's still useful sometimes.
-
+3. Prior to bzr 2.4, we sometimes used to raise warnings from del methods
+   that the object was not cleaned up or closed.  We no longer do this:
+   failure to close the object doesn't cause a test failure; the warning
+   appears an arbitrary long time after the problem occurred (the object
+   being leaked); merely having a del method inhibits Python gc; the
+   warnings appear to users and upset them; they can also break tests that
+   are checking what appears on stderr.
+   
+In short, just don't use ``__del__``.
 
 Cleanup methods
 ===============