[Zodb-checkins] SVN: ZODB/branches/3.4/src/transaction/ Fixed a bug in savepoint rollback. It's not enough to rollback

[Jim Fulton]

Log message for revision 30165:
  Fixed a bug in savepoint rollback.  It's not enough to rollback
  just the savepoint being rolled back because later savepoints
  might involved data managers that hadn't joined when the savepoint
  being rolled back was created.
  
  Now, when a data manager joins and we have savepoints, we create a
  data manager savepoint for the new data manager and add the
  datamanager savepoint to all previous transaction savepoints.  Note
  that this data manager savepoint can be a special savepoint that just
  calls abort on the data manager when it is rolled back.
  

Changed:
  U   ZODB/branches/3.4/src/transaction/_transaction.py
  U   ZODB/branches/3.4/src/transaction/savepoint.txt
  U   ZODB/branches/3.4/src/transaction/tests/test_savepoint.py

-=-
Modified: ZODB/branches/3.4/src/transaction/_transaction.py
===================================================================
--- ZODB/branches/3.4/src/transaction/_transaction.py	2005-04-25 15:16:20 UTC (rev 30164)
+++ ZODB/branches/3.4/src/transaction/_transaction.py	2005-04-25 16:29:28 UTC (rev 30165)
@@ -264,9 +264,7 @@
             self._prior_operation_failed() # doesn't return, it raises
 
         try:
-            savepoint = Savepoint(self, optimistic)
-            for resource in self._resources:
-                savepoint.join(resource)
+            savepoint = Savepoint(self, optimistic, *self._resources)
         except:
             self._cleanup(self._resources)
             self._saveCommitishError() # reraises!
@@ -598,25 +596,42 @@
     """
     interface.implements(interfaces.ISavepoint)
 
-    def __init__(self, transaction, optimistic):
+    def __init__(self, transaction, optimistic, *resources):
         self.transaction = transaction
-        self._savepoints = []
+        self._savepoints = savepoints = []
         self.valid = True
         self.next = self.previous = None
         self.optimistic = optimistic
+
+        for datamanager in resources:
+            try:
+                savepoint = datamanager.savepoint
+            except AttributeError:
+                if not self.optimistic:
+                    raise TypeError("Savepoints unsupported", datamanager)
+                savepoint = NoRollbackSavepoint(datamanager)
+            else:
+                savepoint = savepoint()
+                
+            savepoints.append(savepoint)
     
     def join(self, datamanager):
-        try:
-            savepoint = datamanager.savepoint
-        except AttributeError:
-            if not self.optimistic:
-                raise TypeError("Savepoints unsupported", datamanager)
-            savepoint = NoRollbackSavepoint(datamanager)
-        else:
-            savepoint = savepoint()
-                
-        self._savepoints.append(savepoint)
+        
+        # A data manager has joined a transaction *after* a savepoint
+        # was created.  A couple of things are different in this case:
+        
+        # 1. We need to add it's savepoint to all previous savepoints.
+        # so that if they are rolled back, we roll this was back too.
+        
+        # 2. We don't actualy need to ask it for a savepoint.  Because
+        # is just joining, then we can abort it if there is an error,
+        # so we use an AbortSavepoint.
 
+        savepoint = AbortSavepoint(datamanager, self.transaction)
+        while self is not None:
+            self._savepoints.append(savepoint)
+            self = self.previous
+
     def rollback(self):
         if not self.valid:
             raise interfaces.InvalidSavepointRollbackError
@@ -638,6 +653,15 @@
         if self.previous is not None:
             self.previous._invalidate_previous()
 
+class AbortSavepoint:
+
+    def __init__(self, datamanager, transaction):
+        self.datamanager = datamanager
+        self.transaction = transaction
+
+    def rollback(self):
+        self.datamanager.abort(self.transaction)
+
 class NoRollbackSavepoint:
 
     def __init__(self, datamanager):

Modified: ZODB/branches/3.4/src/transaction/savepoint.txt
===================================================================
--- ZODB/branches/3.4/src/transaction/savepoint.txt	2005-04-25 15:16:20 UTC (rev 30164)
+++ ZODB/branches/3.4/src/transaction/savepoint.txt	2005-04-25 16:29:28 UTC (rev 30165)
@@ -201,7 +201,7 @@
     >>> transaction.abort()
     
 However, a flag can be passed to the transaction savepoint method to
-indicate that databases without savepoint support should be tolderated
+indicate that databases without savepoint support should be tolerated
 until a savepoint is roled back.  This allows transactions to proceed
 is there are no reasons to roll back:
 
@@ -212,8 +212,8 @@
     >>> dm_no_sp['name']
     'sue'
 
+    >>> dm_no_sp['name'] = 'sam'
     >>> savepoint = transaction.savepoint(1)
-    >>> dm_no_sp['name'] = 'sam'
     >>> savepoint.rollback()
     Traceback (most recent call last):
     ...

Modified: ZODB/branches/3.4/src/transaction/tests/test_savepoint.py
===================================================================
--- ZODB/branches/3.4/src/transaction/tests/test_savepoint.py	2005-04-25 15:16:20 UTC (rev 30164)
+++ ZODB/branches/3.4/src/transaction/tests/test_savepoint.py	2005-04-25 16:29:28 UTC (rev 30165)
@@ -19,9 +19,49 @@
 from zope.testing import doctest
 
 
+def testRollbackRollsbackDataManagersThatJoinedLater():
+    """
+
+A savepoint needs to not just rollback it's savepoints, but needs to
+rollback savepoints for data managers that joined savepoints after the
+savepoint:
+
+    >>> import transaction.tests.savepointsample
+    >>> dm = transaction.tests.savepointsample.SampleSavepointDataManager()
+    >>> dm['name'] = 'bob'
+    >>> sp1 = transaction.savepoint()
+    >>> dm['job'] = 'geek'
+    >>> sp2 = transaction.savepoint()
+    >>> dm['salary'] = 'fun'    
+    >>> dm2 = transaction.tests.savepointsample.SampleSavepointDataManager()
+    >>> dm2['name'] = 'sally'
+
+    >>> 'name' in dm
+    True
+    >>> 'job' in dm
+    True
+    >>> 'salary' in dm
+    True
+    >>> 'name' in dm2
+    True
+
+    >>> sp1.rollback()
+
+    >>> 'name' in dm
+    True
+    >>> 'job' in dm
+    False
+    >>> 'salary' in dm
+    False
+    >>> 'name' in dm2
+    False
+
+"""
+
 def test_suite():
     return unittest.TestSuite((
         doctest.DocFileSuite('../savepoint.txt'),
+        doctest.DocTestSuite(),
         ))
 
 if __name__ == '__main__':