[Mailman-Developers] problem with view other subscriptions..

[Harald Meland]

--=-=-=

[Barry A. Warsaw]

>     HM> As I see it, it is more important for the lock file to never
>     HM> have a link count that is neither 0 or 2 than it is to make
>     HM> sure there are no tempfile turds.  This implies that the real
>     HM> lock file should be unlink()ed before the tempfile, and not
>     HM> the other way around.  Here's a (untested) patch (which also
>     HM> touches on some other issues I noticed while I was at it :):
> 
> Thinking about this more, I disagree.  I think it is perfectly fine
> to expect the linkcount to be 0, 1, or 2, but never greater than 2.

I still don't see why there's a need for the "intermediate" state
(neither fully locked nor fully unlocked) with the shared lockfile
having a link count of 1.  I think it just makes the locking logic
more complicated to understand.

Also keep in mind that calling __linkcount() is not an atomic
operation.  Doing state tests on the return value of multiple calls to
this method makes my race-condition-meter howl rather irritatingly. :)

> To me, the more important invariant is that if the global lockfile
> exists, somebody else has the lock.  Changing the order of unlinks
> breaks this invariant.

Does it?  The shared lockfile is, in the current CVS code, unlinked
from two places.

Obviously, it needs to be unlinked when someone calls unlock().  By
doing the unlink of the shared lockfile before unlinking the tempfile,
you release the lock a tad earlier -- but that's not of any
consequence, as you were going to release the lock before returning
anyway.

The other method that currently unlinks the shared lockfile is
__break().  The purpose of that method is to remove lockfiles that
some other process has failed to release within the lock's lifetime.
Thus, this method breaks the invariant you mention *by design*.

By reversing the order of unlinks, we reduce the number of lock
failure modes.  In my book, that's a good thing. :)

Here's a quick patch to illustrate how I believe things could be
changed without breaking the "lockfile exists <=> lock is taken"
invariant:


--=-=-=
Content-Type: text/x-patch
Content-Disposition: inline

Index: LockFile.py
===================================================================
RCS file: /cvsroot/mailman/mailman/Mailman/LockFile.py,v
retrieving revision 1.18
diff -u -r1.18 LockFile.py
--- LockFile.py	2000/06/01 22:08:10	1.18
+++ LockFile.py	2000/06/04 21:14:12
@@ -300,18 +300,21 @@
         islocked = self.locked()
         if not islocked and not unconditionally:
             raise NotLockedError
-        # Remove our tempfile
-        try:
-            os.unlink(self.__tmpfname)
-        except OSError, e:
-            if e.errno <> errno.ENOENT: raise
         # If we owned the lock, remove the global file, relinquishing it.
+        # By removing the lockfile before the tempfile, we ensure that
+        # the link count of the lockfile always will be either 0 or 2,
+        # and thus reduce the number of lock failure modes.
         if islocked:
             try:
                 os.unlink(self.__lockfile)
             except OSError, e:
                 if e.errno <> errno.ENOENT: raise
         self.__writelog('unlocked')
+        # Remove our tempfile
+        try:
+            os.unlink(self.__tmpfname)
+        except OSError, e:
+            if e.errno <> errno.ENOENT: raise
 
     def locked(self):
         """Returns 1 if we own the lock, 0 if we do not.
@@ -399,18 +402,19 @@
             self.__touch(self.__lockfile)
         except OSError, e:
             if e.errno <> errno.EPERM: raise
+        # Try getting the name of the old winner's temp file.
+        winner = self.__read()
+        # Remove the global lockfile.  This actually breaks the lock.
+        try:
+            os.unlink(self.__lockfile)
+        except OSError, e:
+            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,
         # but will leave temp file turds laying around, a minor inconvenience.
-        winner = self.__read()
         if winner:
             os.unlink(winner)
-        # Now remove the global lockfile, which actually breaks the lock.
-        try:
-            os.unlink(self.__lockfile)
-        except OSError, e:
-            if e.errno <> errno.ENOENT: raise
 
     def __sleep(self):
         interval = random.random() * 2.0 + 0.01

--=-=-=
Content-Disposition: inline

-- 
Harald

--=-=-=--