Moved the session.expire() to the MailList.Load() method, via

DBContext.api_load().  This is where it really ought to be based on the
internal semantics of .Load()/.Lock().

MailListMapperExtension.populate_instance(): Checking the state of the isnew
flag is not sufficient to know whether the MailList object we're getting is
brand spankin' new or not.  It turns out that when we session.expire() the
MailList object, the next time SA loads this from the db, the
populate_instance() will get called with isnew=True, even though the object
really isn't new.  Instead, check to make sure InitTempVars() isn't
incorrectly called twice.  Note that I might move this test, but I wanted to
check in something that works, and then see if this is what we expect from the
SA guys (this flag appears underdocumented).

LockFile.py: Add some additional debugging.

4 files changed, 15 insertions, 2 deletions

diff --git a/Mailman/LockFile.py b/Mailman/LockFile.py
index d13699ec4..20f6349ef 100644
--- a/Mailman/LockFile.py
+++ b/Mailman/LockFile.py
@@ -313,10 +313,12 @@ class LockFile:
         return self._read() == self._tmpfname
 
     def finalize(self):
+        log.debug('finalize')
         self.unlock(unconditionally=True)
 
     # XXX Can we just get rid of __del__()?
     def __del__(self):
+        log.debug('__del__')
         if self._owned:
             self.finalize()
 
diff --git a/Mailman/MailList.py b/Mailman/MailList.py
index fcdbca9a6..e2979b692 100644
--- a/Mailman/MailList.py
+++ b/Mailman/MailList.py
@@ -586,6 +586,7 @@ class MailList(object, HTMLFormatter, Deliverer, ListAdmin,
             fqdn_listname = self.fqdn_listname
         if not Utils.list_exists(fqdn_listname):
             raise Errors.MMUnknownListError(fqdn_listname)
+        database.load(self)
         self._memberadaptor.load()
         if check_version:
             # XXX for now disable version checks.  We'll fold this into schema
diff --git a/Mailman/database/dbcontext.py b/Mailman/database/dbcontext.py
index 306e0b2bb..3482122c9 100644
--- a/Mailman/database/dbcontext.py
+++ b/Mailman/database/dbcontext.py
@@ -129,7 +129,6 @@ class DBContext(object):
 
     # Higher level interface
     def api_lock(self, mlist):
-        self.session.expire(mlist)
         # Don't try to re-lock a list
         if mlist.fqdn_listname in self._mlist_txns:
             return
@@ -152,6 +151,11 @@ class DBContext(object):
             txn.rollback()
         del mlist._txnkey
 
+    def api_load(self, mlist):
+        # Mark the MailList object such that future attribute accesses will
+        # refresh from the database.
+        self.session.expire(mlist)
+
     def api_save(self, mlist):
         # When dealing with MailLists, .Save() will always be followed by
         # .Unlock().  However lists can also be unlocked without saving.  But
diff --git a/Mailman/database/listdata.py b/Mailman/database/listdata.py
index 92a2d070a..05ea967ff 100644
--- a/Mailman/database/listdata.py
+++ b/Mailman/database/listdata.py
@@ -173,7 +173,13 @@ def make_table(metadata, tables):
 
 class MailListMapperExtension(MapperExtension):
     def populate_instance(self, mapper, context, row, mlist, ikey, isnew):
-        if isnew:
+        # isnew doesn't really seem to give us what we want.  Specifically, if
+        # we expire the MailList object from the session, we'll get called
+        # with populate_instance(..., isnew=True) when the object is reloaded
+        # from the database.  We'll still check isnew, but we'll also check to
+        # make sure there's no _memberadaptor attribute, since that is set in
+        # the InitTempVars() method.
+        if isnew and not hasattr(mlist, '_memberadaptor'):
             # Get the list name and host name -- which are required by
             # InitTempVars() from the row data.
             list_name = row['listdata_list_name']