1 files changed, 113 insertions, 145 deletions

diff --git a/Mailman/LockFile.py b/Mailman/LockFile.py
index cc010ce5e..06d5c5f8e 100644
--- a/Mailman/LockFile.py
+++ b/Mailman/LockFile.py
@@ -12,7 +12,8 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301,
+# USA.
 
 """Portable, NFS-safe file locking with timeouts.
 
@@ -44,12 +45,6 @@ lifetime has expired.  Too long and other processes will hang; too short and
 you'll end up trampling on existing process locks -- and possibly corrupting
 data.  In a distributed (NFS) environment, you also need to make sure that
 your clocks are properly synchronized.
-
-Locks can also log their state to a log file.  When running under Mailman, the
-log file is placed in a Mailman-specific location, otherwise, the log file is
-called `LockFile.log' and placed in the temp directory (calculated from
-tempfile.mktemp()).
-
 """
 
 # This code has undergone several revisions, with contributions from Barry
@@ -63,42 +58,16 @@ import time
 import errno
 import random
 import socket
+import logging
 import traceback
 
-from stat import ST_NLINK, ST_MTIME
-
 # Units are floating-point seconds.
 DEFAULT_LOCK_LIFETIME  = 15
 # Allowable a bit of clock skew
 CLOCK_SLOP = 10
-
-
-
-# Figure out what logfile to use.  This is different depending on whether
-# we're running in a Mailman context or not.
-_logfile = None
-
-def _get_logfile():
-    global _logfile
-    if _logfile is None:
-        try:
-            from Mailman.Logging.StampedLogger import StampedLogger
-            _logfile = StampedLogger('locks')
-        except ImportError:
-            # not running inside Mailman
-            import tempfile
-            dir = os.path.split(tempfile.mktemp())[0]
-            path = os.path.join(dir, 'LockFile.log')
-            # open in line-buffered mode
-            class SimpleUserFile:
-                def __init__(self, path):
-                    self.__fp = open(path, 'a', 1)
-                    self.__prefix = '(%d) ' % os.getpid()
-                def write(self, msg):
-                    now = '%.3f' % time.time()
-                    self.__fp.write(self.__prefix + now + ' ' + msg)
-            _logfile = SimpleUserFile(path)
-    return _logfile
+# This is appropriate for Mailman, but you may want to change this if you're
+# using this code outside Mailman.
+log = logging.getLogger('mailman.locks')
 
 
 
@@ -122,13 +91,12 @@ class LockFile:
 
     This class supports the following methods:
 
-    __init__(lockfile[, lifetime[, withlogging]]):
+    __init__(lockfile[, lifetime]):
         Create the resource lock using lockfile as the global lock file.  Each
         process laying claim to this resource lock will create their own
         temporary lock files based on the path specified by lockfile.
         Optional lifetime is the number of seconds the process expects to hold
-        the lock.  Optional withlogging, when true, turns on lockfile logging
-        (see the module docstring for details).
+        the lock.
 
     set_lifetime(lifetime):
         Set a new lock lifetime.  This takes affect the next time the file is
@@ -160,16 +128,14 @@ class LockFile:
         conditions, this refreshes the lock (on set locks).
 
     """
-    # BAW: We need to watch out for two lock objects in the same process
+    # XXX We need to watch out for two lock objects in the same process
     # pointing to the same lock file.  Without this, if you lock lf1 and do
     # not lock lf2, lf2.locked() will still return true.  NOTE: this gimmick
     # probably does /not/ work in a multithreaded world, but we don't have to
     # worry about that, do we? <1 wink>.
     COUNTER = 0
 
-    def __init__(self, lockfile,
-                 lifetime=DEFAULT_LOCK_LIFETIME,
-                 withlogging=False):
+    def __init__(self, lockfile, lifetime=DEFAULT_LOCK_LIFETIME):
         """Create the resource lock using lockfile as the global lock file.
 
         Each process laying claim to this resource lock will create their own
@@ -177,25 +143,22 @@ class LockFile:
         Optional lifetime is the number of seconds the process expects to hold
         the lock.  Optional withlogging, when true, turns on lockfile logging
         (see the module docstring for details).
-
         """
-        self.__lockfile = lockfile
-        self.__lifetime = lifetime
+        self._lockfile = lockfile
+        self._lifetime = lifetime
         # This works because we know we're single threaded
-        self.__counter = LockFile.COUNTER
+        self._counter = LockFile.COUNTER
         LockFile.COUNTER += 1
-        self.__tmpfname = '%s.%s.%d.%d' % (
-            lockfile, socket.gethostname(), os.getpid(), self.__counter)
-        self.__withlogging = withlogging
-        self.__logprefix = os.path.split(self.__lockfile)[1]
+        self._tmpfname = '%s.%s.%d.%d' % (
+            lockfile, socket.gethostname(), os.getpid(), self._counter)
         # For transferring ownership across a fork.
-        self.__owned = True
+        self._owned = True
 
     def __repr__(self):
         return '<LockFile %s: %s [%s: %ssec] pid=%s>' % (
-            id(self), self.__lockfile,
+            id(self), self._lockfile,
             self.locked() and 'locked' or 'unlocked',
-            self.__lifetime, os.getpid())
+            self._lifetime, os.getpid())
 
     def set_lifetime(self, lifetime):
         """Set a new lock lifetime.
@@ -203,11 +166,11 @@ class LockFile:
         This takes affect the next time the file is locked, but does not
         refresh a locked file.
         """
-        self.__lifetime = lifetime
+        self._lifetime = lifetime
 
     def get_lifetime(self):
         """Return the lock's lifetime."""
-        return self.__lifetime
+        return self._lifetime
 
     def refresh(self, newlifetime=None, unconditionally=False):
         """Refreshes the lifetime of a locked file.
@@ -221,7 +184,7 @@ class LockFile:
             self.set_lifetime(newlifetime)
         # Do we have the lock?  As a side effect, this refreshes the lock!
         if not self.locked() and not unconditionally:
-            raise NotLockedError, '%s: %s' % (repr(self), self.__read())
+            raise NotLockedError('%s: %s' % (repr(self), self._read()))
 
     def lock(self, timeout=0):
         """Acquire the lock.
@@ -235,35 +198,35 @@ class LockFile:
             timeout_time = time.time() + timeout
         # Make sure my temp lockfile exists, and that its contents are
         # up-to-date (e.g. the temp file name, and the lock lifetime).
-        self.__write()
-        # TBD: This next call can fail with an EPERM.  I have no idea why, but
+        self._write()
+        # XXX This next call can fail with an EPERM.  I have no idea why, but
         # I'm nervous about wrapping this in a try/except.  It seems to be a
         # very rare occurence, only happens from cron, and (only?) on Solaris
         # 2.6.
-        self.__touch()
-        self.__writelog('laying claim')
+        self._touch()
+        log.debug('laying claim')
         # for quieting the logging output
         loopcount = -1
         while True:
             loopcount += 1
             # Create the hard link and test for exactly 2 links to the file
             try:
-                os.link(self.__tmpfname, self.__lockfile)
+                os.link(self._tmpfname, self._lockfile)
                 # If we got here, we know we know we got the lock, and never
                 # had it before, so we're done.  Just touch it again for the
                 # fun of it.
-                self.__writelog('got the lock')
-                self.__touch()
+                log.debug('got the lock')
+                self._touch()
                 break
             except OSError, e:
                 # The link failed for some reason, possibly because someone
                 # else already has the lock (i.e. we got an EEXIST), or for
                 # some other bizarre reason.
                 if e.errno == errno.ENOENT:
-                    # TBD: in some Linux environments, it is possible to get
+                    # XXX in some Linux environments, it is possible to get
                     # an ENOENT, which is truly strange, because this means
-                    # that self.__tmpfname doesn't exist at the time of the
-                    # os.link(), but self.__write() is supposed to guarantee
+                    # that self._tmpfname doesn't exist at the time of the
+                    # os.link(), but self._write() is supposed to guarantee
                     # that this happens!  I don't honestly know why this
                     # happens, but for now we just say we didn't acquire the
                     # lock, and try again next time.
@@ -271,40 +234,37 @@ class LockFile:
                 elif e.errno <> errno.EEXIST:
                     # Something very bizarre happened.  Clean up our state and
                     # pass the error on up.
-                    self.__writelog('unexpected link error: %s' % e,
-                                    important=True)
-                    os.unlink(self.__tmpfname)
+                    log.exception('unexpected link')
+                    os.unlink(self._tmpfname)
                     raise
-                elif self.__linkcount() <> 2:
+                elif self._linkcount() <> 2:
                     # Somebody's messin' with us!  Log this, and try again
-                    # later.  TBD: should we raise an exception?
-                    self.__writelog('unexpected linkcount: %d' %
-                                    self.__linkcount(), important=True)
-                elif self.__read() == self.__tmpfname:
+                    # later.  XXX should we raise an exception?
+                    log.error('unexpected linkcount: %d', self._linkcount())
+                elif self._read() == self._tmpfname:
                     # It was us that already had the link.
-                    self.__writelog('already locked')
+                    log.debug('already locked')
                     raise AlreadyLockedError
                 # otherwise, someone else has the lock
                 pass
             # We did not acquire the lock, because someone else already has
             # it.  Have we timed out in our quest for the lock?
             if timeout and timeout_time < time.time():
-                os.unlink(self.__tmpfname)
-                self.__writelog('timed out')
+                os.unlink(self._tmpfname)
+                log.error('timed out')
                 raise TimeOutError
             # Okay, we haven't timed out, but we didn't get the lock.  Let's
             # find if the lock lifetime has expired.
-            if time.time() > self.__releasetime() + CLOCK_SLOP:
+            if time.time() > self._releasetime() + CLOCK_SLOP:
                 # Yes, so break the lock.
-                self.__break()
-                self.__writelog('lifetime has expired, breaking',
-                                important=True)
+                self._break()
+                log.error('lifetime has expired, breaking')
             # Okay, someone else has the lock, our claim hasn't timed out yet,
             # and the expected lock lifetime hasn't expired yet.  So let's
             # wait a while for the owner of the lock to give it up.
             elif not loopcount % 100:
-                self.__writelog('waiting for claim')
-            self.__sleep()
+                log.debug('waiting for claim')
+            self._sleep()
 
     def unlock(self, unconditionally=False):
         """Unlock the lock.
@@ -319,15 +279,17 @@ class LockFile:
         # If we owned the lock, remove the global file, relinquishing it.
         if islocked:
             try:
-                os.unlink(self.__lockfile)
+                os.unlink(self._lockfile)
             except OSError, e:
-                if e.errno <> errno.ENOENT: raise
+                if e.errno <> errno.ENOENT:
+                    raise
         # Remove our tempfile
         try:
-            os.unlink(self.__tmpfname)
+            os.unlink(self._tmpfname)
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
-        self.__writelog('unlocked')
+            if e.errno <> errno.ENOENT:
+                raise
+        log.debug('unlocked')
 
     def locked(self):
         """Return true if we own the lock, false if we do not.
@@ -337,7 +299,7 @@ class LockFile:
         """
         # Discourage breaking the lock for a while.
         try:
-            self.__touch()
+            self._touch()
         except OSError, e:
             if e.errno == errno.EPERM:
                 # We can't touch the file because we're not the owner.  I
@@ -345,16 +307,17 @@ class LockFile:
                 return False
             else:
                 raise
-        # TBD: can the link count ever be > 2?
-        if self.__linkcount() <> 2:
+        # XXX Can the link count ever be > 2?
+        if self._linkcount() <> 2:
             return False
-        return self.__read() == self.__tmpfname
+        return self._read() == self._tmpfname
 
     def finalize(self):
         self.unlock(unconditionally=True)
 
+    # XXX Can we just get rid of __del__()?
     def __del__(self):
-        if self.__owned:
+        if self._owned:
             self.finalize()
 
     # Use these only if you're transfering ownership to a child process across
@@ -362,97 +325,99 @@ class LockFile:
     # _transfer_to() is called in the parent, passing in the pid of the child.
     # _take_possession() is called in the child, and blocks until the parent
     # has transferred possession to the child.  _disown() is used to set the
-    # __owned flag to false, and it is a disgusting wart necessary to make
+    # _owned flag to false, and it is a disgusting wart necessary to make
     # forced lock acquisition work in mailmanctl. :(
     def _transfer_to(self, pid):
         # First touch it so it won't get broken while we're fiddling about.
-        self.__touch()
+        self._touch()
         # Find out current claim's temp filename
-        winner = self.__read()
+        winner = self._read()
         # Now twiddle ours to the given pid
-        self.__tmpfname = '%s.%s.%d' % (
-            self.__lockfile, socket.gethostname(), pid)
+        self._tmpfname = '%s.%s.%d' % (
+            self._lockfile, socket.gethostname(), pid)
         # Create a hard link from the global lock file to the temp file.  This
         # actually does things in reverse order of normal operation because we
         # know that lockfile exists, and tmpfname better not!
-        os.link(self.__lockfile, self.__tmpfname)
+        os.link(self._lockfile, self._tmpfname)
         # Now update the lock file to contain a reference to the new owner
-        self.__write()
+        self._write()
         # Toggle off our ownership of the file so we don't try to finalize it
         # in our __del__()
-        self.__owned = False
+        self._owned = False
         # Unlink the old winner, completing the transfer
         os.unlink(winner)
         # And do some sanity checks
-        assert self.__linkcount() == 2
+        assert self._linkcount() == 2
         assert self.locked()
-        self.__writelog('transferred the lock')
+        log.debug('transferred the lock')
 
     def _take_possession(self):
-        self.__tmpfname = tmpfname = '%s.%s.%d' % (
-            self.__lockfile, socket.gethostname(), os.getpid())
+        self._tmpfname = tmpfname = '%s.%s.%d' % (
+            self._lockfile, socket.gethostname(), os.getpid())
         # Wait until the linkcount is 2, indicating the parent has completed
         # the transfer.
-        while self.__linkcount() <> 2 or self.__read() <> tmpfname:
+        while self._linkcount() <> 2 or self._read() <> tmpfname:
             time.sleep(0.25)
-        self.__writelog('took possession of the lock')
+        log.debug('took possession of the lock')
 
     def _disown(self):
-        self.__owned = False
+        self._owned = False
 
     #
     # Private interface
     #
 
-    def __writelog(self, msg, important=0):
-        if self.__withlogging or important:
-            logf = _get_logfile()
-            logf.write('%s %s\n' % (self.__logprefix, msg))
-            traceback.print_stack(file=logf)
-
-    def __write(self):
+    def _write(self):
         # Make sure it's group writable
         oldmask = os.umask(002)
+        fp = None
         try:
-            fp = open(self.__tmpfname, 'w')
-            fp.write(self.__tmpfname)
-            fp.close()
+            fp = open(self._tmpfname, 'w')
+            fp.write(self._tmpfname)
         finally:
+            if fp:
+                fp.close()
             os.umask(oldmask)
 
-    def __read(self):
+    def _read(self):
         try:
-            fp = open(self.__lockfile)
-            filename = fp.read()
-            fp.close()
+            fp = open(self._lockfile)
+            try:
+                filename = fp.read()
+            finally:
+                fp.close()
             return filename
         except EnvironmentError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
             return None
 
-    def __touch(self, filename=None):
-        t = time.time() + self.__lifetime
+    def _touch(self, filename=None):
+        t = time.time() + self._lifetime
         try:
-            # TBD: We probably don't need to modify atime, but this is easier.
-            os.utime(filename or self.__tmpfname, (t, t))
+            # XXX We probably don't need to modify atime, but this is easier.
+            os.utime(filename or self._tmpfname, (t, t))
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
 
-    def __releasetime(self):
+    def _releasetime(self):
         try:
-            return os.stat(self.__lockfile)[ST_MTIME]
+            return os.stat(self._lockfile).st_mtime
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
             return -1
 
-    def __linkcount(self):
+    def _linkcount(self):
         try:
-            return os.stat(self.__lockfile)[ST_NLINK]
+            return os.stat(self._lockfile).st_nlink
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
             return -1
 
-    def __break(self):
+    def _break(self):
         # First, touch the global lock file.  This reduces but does not
         # eliminate the chance for a race condition during breaking.  Two
         # processes could both pass the test for lock expiry in lock() before
@@ -466,16 +431,18 @@ class LockFile:
         # solve this by set-uid'ing the CGI and mail wrappers, but I don't
         # think it's that big a problem.
         try:
-            self.__touch(self.__lockfile)
+            self._touch(self._lockfile)
         except OSError, e:
-            if e.errno <> errno.EPERM: raise
+            if e.errno <> errno.EPERM:
+                raise
         # Get the name of the old winner's temp file.
-        winner = self.__read()
+        winner = self._read()
         # Remove the global lockfile, which actually breaks the lock.
         try:
-            os.unlink(self.__lockfile)
+            os.unlink(self._lockfile)
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
         # Try to remove the old winner's temp file, since we're assuming the
         # winner process has hung or died.  Don't worry too much if we can't
         # unlink their temp file -- this doesn't wreck the locking algorithm,
@@ -484,9 +451,10 @@ class LockFile:
             if winner:
                 os.unlink(winner)
         except OSError, e:
-            if e.errno <> errno.ENOENT: raise
+            if e.errno <> errno.ENOENT:
+                raise
 
-    def __sleep(self):
+    def _sleep(self):
         interval = random.random() * 2.0 + 0.01
         time.sleep(interval)
 
@@ -496,7 +464,7 @@ class LockFile:
 def _dochild():
     prefix = '[%d]' % os.getpid()
     # Create somewhere between 1 and 1000 locks
-    lockfile = LockFile('/tmp/LockTest', withlogging=True, lifetime=120)
+    lockfile = LockFile('/tmp/LockTest', lifetime=120)
     # Use a lock lifetime of between 1 and 15 seconds.  Under normal
     # situations, Mailman's usage patterns (untested) shouldn't be much longer
     # than this.