Rev 6002: (mbp) remove most __del__ methods (Martin Pool) in file:///home/pqm/archives/thelove/bzr/%2Btrunk/
Canonical.com Patch Queue Manager
pqm at pqm.ubuntu.com
Wed Jun 29 17:20:06 UTC 2011
At file:///home/pqm/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 6002 [merge]
revision-id: pqm at pqm.ubuntu.com-20110629171958-u8a6lkpw5kdnfm55
parent: pqm at pqm.ubuntu.com-20110628191210-bwblsxn26kyu3swl
parent: mbp at canonical.com-20110628214724-io85x1yegw9q2dld
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Wed 2011-06-29 17:19:58 +0000
message:
(mbp) remove most __del__ methods (Martin Pool)
modified:
bzrlib/atomicfile.py atomicfile.py-20050509044450-dbd24e6c564f7c66
bzrlib/lock.py lock.py-20050527050856-ec090bb51bc03349
bzrlib/lockable_files.py control_files.py-20051111201905-bb88546e799d669f
bzrlib/tests/test_fifo_cache.py test_fifo_cache.py-20081209212307-31ffjwvteyvmydnf-2
bzrlib/transport/memory.py memory.py-20051016101338-cd008dbdf69f04fc
bzrlib/transport/sftp.py sftp.py-20051019050329-ab48ce71b7e32dfe
doc/developers/code-style.txt codestyle.txt-20100515105711-133ealf7ereiq2eq-1
=== modified file 'bzrlib/atomicfile.py'
--- a/bzrlib/atomicfile.py 2011-05-20 14:46:02 +0000
+++ b/bzrlib/atomicfile.py 2011-06-28 21:47:24 +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'
--- a/bzrlib/lock.py 2011-03-30 11:45:54 +0000
+++ b/bzrlib/lock.py 2011-06-28 21:47:24 +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'
--- a/bzrlib/lockable_files.py 2011-04-07 10:36:24 +0000
+++ b/bzrlib/lockable_files.py 2011-06-28 21:47:24 +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'
--- a/bzrlib/tests/test_fifo_cache.py 2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/test_fifo_cache.py 2011-06-14 01:43:10 +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'
--- a/bzrlib/transport/memory.py 2010-02-23 07:43:11 +0000
+++ b/bzrlib/transport/memory.py 2011-06-28 21:47:24 +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'
--- a/bzrlib/transport/sftp.py 2011-04-20 14:27:19 +0000
+++ b/bzrlib/transport/sftp.py 2011-06-28 21:47:24 +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'
--- a/doc/developers/code-style.txt 2010-12-14 23:39:41 +0000
+++ b/doc/developers/code-style.txt 2011-06-28 21:45:01 +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
===============
More information about the bazaar-commits
mailing list