SVN: ZODB/trunk/src/ Bug fixed:

Jim Fulton <[email protected]>
Newsgroups gmane.comp.python.zope.zodb.cvs
Message-ID <[email protected]>
Log message for revision 107889:
  Bug fixed:
  
  - The undo implementation was incorrect in ways that could cause
    subtle missbehaviors.
  
  API changes:
  
  - The API for undoing multiple transactions has changed.  To undo
    multiple transactions in a single transaction, pass pass a list of
    transaction identifiers to a database's undo method. Calling a
    database's undo method multiple times in the same transaction now
    raises an exception.
  
  - The storage API (IStorage) has been tightened. Now, storages should
    raise a StorageTransactionError when invalid transactions are passed
    to tpc_begin, tpc_vote, or tpc_finish.
  

Changed:
  U   ZODB/trunk/src/CHANGES.txt
  U   ZODB/trunk/src/ZEO/ClientStorage.py
  U   ZODB/trunk/src/ZEO/tests/ConnectionTests.py
  U   ZODB/trunk/src/ZODB/BaseStorage.py
  U   ZODB/trunk/src/ZODB/DB.py
  U   ZODB/trunk/src/ZODB/DemoStorage.py
  U   ZODB/trunk/src/ZODB/FileStorage/FileStorage.py
  U   ZODB/trunk/src/ZODB/MappingStorage.py
  U   ZODB/trunk/src/ZODB/interfaces.py
  U   ZODB/trunk/src/ZODB/tests/BasicStorage.py
  U   ZODB/trunk/src/ZODB/tests/Synchronization.py
  U   ZODB/trunk/src/ZODB/tests/testZODB.py

-=-
Modified: ZODB/trunk/src/CHANGES.txt
===================================================================
--- ZODB/trunk/src/CHANGES.txt	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/CHANGES.txt	2010-01-08 22:32:35 UTC (rev 107889)
@@ -8,6 +8,16 @@
 New Features
 ------------
 
+- The API for undoing multiple transactions has changed.  To undo
+  multiple transactions in a single transaction, pass pass a list of
+  transaction identifiers to a database's undo method. Calling a
+  database's undo method multiple times in the same transaction now
+  raises an exception.
+
+- The storage API (IStorage) has been tightened. Now, storages should
+  raise a StorageTransactionError when invalid transactions are passed
+  to tpc_begin, tpc_vote, or tpc_finish.
+
 - Broken objects now provide the IBroken interface.
 
 Bugs Fixed
@@ -43,6 +53,9 @@
 
 - C Header files weren't installed correctly.
 
+- The undo implementation was incorrect in ways that could cause
+  subtle missbehaviors.
+
 3.9.3 (2009-10-23)
 ==================
 

Modified: ZODB/trunk/src/ZEO/ClientStorage.py
===================================================================
--- ZODB/trunk/src/ZEO/ClientStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZEO/ClientStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -1075,7 +1075,8 @@
     def tpc_vote(self, txn):
         """Storage API: vote on a transaction."""
         if txn is not self._transaction:
-            return
+            raise POSException.StorageTransactionError(
+                "tpc_vote called with wrong transaction")
         self._server.vote(id(txn))
         return self._check_serials()
 
@@ -1094,7 +1095,9 @@
             # must be ignored.
             if self._transaction == txn:
                 self._tpc_cond.release()
-                return
+                raise POSException.StorageTransactionError(
+                    "Duplicate tpc_begin calls for same transaction")
+
             self._tpc_cond.wait(30)
         self._transaction = txn
         self._tpc_cond.release()
@@ -1148,7 +1151,8 @@
     def tpc_finish(self, txn, f=None):
         """Storage API: finish a transaction."""
         if txn is not self._transaction:
-            return
+            raise POSException.StorageTransactionError(
+                "tpc_finish called with wrong transaction")
         self._load_lock.acquire()
         try:
             if self._midtxn_disconnect:

Modified: ZODB/trunk/src/ZEO/tests/ConnectionTests.py
===================================================================
--- ZODB/trunk/src/ZEO/tests/ConnectionTests.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZEO/tests/ConnectionTests.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -1093,7 +1093,8 @@
         self.assertRaises(ConflictError, storage.tpc_vote, t)
         # Even aborting won't help.
         storage.tpc_abort(t)
-        storage.tpc_finish(t)
+        self.assertRaises(ZODB.POSException.StorageTransactionError,
+                          storage.tpc_finish, t)
         # Try again.
         obj.value = 10
         t = Transaction()
@@ -1103,7 +1104,7 @@
         self.assertRaises(ConflictError, storage.tpc_vote, t)
         # Abort this one and try a transaction that should succeed.
         storage.tpc_abort(t)
-        storage.tpc_finish(t)
+        
         # Now do a store.
         obj.value = 11
         t = Transaction()

Modified: ZODB/trunk/src/ZODB/BaseStorage.py
===================================================================
--- ZODB/trunk/src/ZODB/BaseStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/BaseStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -221,7 +221,8 @@
         self._lock_acquire()
         try:
             if self._transaction is transaction:
-                return
+                raise POSException.StorageTransactionError(
+                    "Duplicate tpc_begin calls for same transaction")
             self._lock_release()
             self._commit_lock_acquire()
             self._lock_acquire()
@@ -264,7 +265,8 @@
         self._lock_acquire()
         try:
             if transaction is not self._transaction:
-                return
+                raise POSException.StorageTransactionError(
+                    "tpc_vote called with wrong transaction")
             self._vote()
         finally:
             self._lock_release()
@@ -284,7 +286,8 @@
         self._lock_acquire()
         try:
             if transaction is not self._transaction:
-                return
+                raise POSException.StorageTransactionError(
+                    "tpc_finish called with wrong transaction")
             try:
                 if f is not None:
                     f(self._tid)

Modified: ZODB/trunk/src/ZODB/DB.py
===================================================================
--- ZODB/trunk/src/ZODB/DB.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/DB.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -896,7 +896,7 @@
         finally:
             self._r()
 
-    def undo(self, id, txn=None):
+    def undo(self, ids, txn=None):
         """Undo a transaction identified by id.
 
         A transaction can be undone if all of the objects involved in
@@ -909,13 +909,16 @@
         transaction id used by other methods; it is unique to undo().
 
         :Parameters:
-          - `id`: a storage-specific transaction identifier
+          - `ids`: a sequence of storage-specific transaction identifiers
+                   or a single transaction identifier
           - `txn`: transaction context to use for undo().
             By default, uses the current transaction.
         """
         if txn is None:
             txn = transaction.get()
-        txn.register(TransactionalUndo(self, id))
+        if isinstance(ids, basestring):
+            ids = [ids]
+        txn.join(TransactionalUndo(self, ids))
 
     def transaction(self):
         return ContextManager(self)
@@ -943,61 +946,42 @@
 resource_counter_lock = threading.Lock()
 resource_counter = 0
 
-class ResourceManager(object):
-    """Transaction participation for an undo resource."""
+class TransactionalUndo(object):
 
-    # XXX This implementation is broken.  Subclasses invalidate oids
-    # in their commit calls. Invalidations should not be sent until
-    # tpc_finish is called.  In fact, invalidations should be sent to
-    # the db *while* tpc_finish is being called on the storage.
-
-    def __init__(self, db):
+    def __init__(self, db, tids):
         self._db = db
-        # Delegate the actual 2PC methods to the storage
-        self.tpc_vote = self._db.storage.tpc_vote
-        self.tpc_finish = self._db.storage.tpc_finish
-        self.tpc_abort = self._db.storage.tpc_abort
+        self._storage = db.storage
+        self._tids = tids
+        self._oids = set()
 
-        # Get a number from a simple thread-safe counter, then
-        # increment it, for the purpose of sorting ResourceManagers by
-        # creation order.  This ensures that multiple ResourceManagers
-        # within a transaction commit in a predictable sequence.
-        resource_counter_lock.acquire()
-        try:
-            global resource_counter
-            self._count = resource_counter
-            resource_counter += 1
-        finally:
-            resource_counter_lock.release()
+    def abort(self, transaction):
+        pass
 
-    def sortKey(self):
-        return "%s:%016x" % (self._db.storage.sortKey(), self._count)
+    def tpc_begin(self, transaction):
+        self._storage.tpc_begin(transaction)
 
-    def tpc_begin(self, txn, sub=False):
-        if sub:
-            raise ValueError("doesn't support sub-transactions")
-        self._db.storage.tpc_begin(txn)
+    def commit(self, transaction):
+        for tid in self._tids:
+            result = self._storage.undo(tid, transaction)
+            if result:
+                self._oids.update(result[1])
 
-    # The object registers itself with the txn manager, so the ob
-    # argument to the methods below is self.
+    def tpc_vote(self, transaction):
+        for oid, _ in  self._storage.tpc_vote(transaction) or ():
+            self._oids.add(oid)
 
-    def abort(self, obj, txn):
-        raise NotImplementedError
+    def tpc_finish(self, transaction):
+        self._storage.tpc_finish(
+            transaction,
+            lambda tid: self._db.invalidate(tid, self._oids)
+            )
 
-    def commit(self, obj, txn):
-        raise NotImplementedError
+    def tpc_abort(self, transaction):
+        self._storage.tpc_abort(transaction)
 
-class TransactionalUndo(ResourceManager):
+    def sortKey(self):
+        return "%s:%s" % (self._storage.sortKey(), id(self))
 
-    def __init__(self, db, tid):
-        super(TransactionalUndo, self).__init__(db)
-        self._tid = tid
-
-    def commit(self, ob, t):
-        # XXX see XXX in ResourceManager
-        tid, oids = self._db.storage.undo(self._tid, t)
-        self._db.invalidate(tid, dict.fromkeys(oids, 1))
-
 def connection(*args, **kw):
     db = DB(*args, **kw)
     conn = db.open()

Modified: ZODB/trunk/src/ZODB/DemoStorage.py
===================================================================
--- ZODB/trunk/src/ZODB/DemoStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/DemoStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -309,7 +309,8 @@
     def tpc_begin(self, transaction, *a, **k):
         # The tid argument exists to support testing.
         if transaction is self._transaction:
-            return
+            raise ZODB.POSException.StorageTransactionError(
+                "Duplicate tpc_begin calls for same transaction")
         self._lock_release()
         self._commit_lock.acquire()
         self._lock_acquire()
@@ -320,7 +321,8 @@
     @ZODB.utils.locked
     def tpc_finish(self, transaction, func = lambda tid: None):
         if (transaction is not self._transaction):
-            return
+            raise ZODB.POSException.StorageTransactionError(
+                "tpc_finish called with wrong transaction")
         self._issued_oids.difference_update(self._stored_oids)
         self._stored_oids = set()
         self._transaction = None

Modified: ZODB/trunk/src/ZODB/FileStorage/FileStorage.py
===================================================================
--- ZODB/trunk/src/ZODB/FileStorage/FileStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/FileStorage/FileStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -705,7 +705,8 @@
         self._lock_acquire()
         try:
             if transaction is not self._transaction:
-                return
+                raise POSException.StorageTransactionError(
+                    "tpc_vote called with wrong transaction")
             dlen = self._tfile.tell()
             if not dlen:
                 return # No data in this trans

Modified: ZODB/trunk/src/ZODB/MappingStorage.py
===================================================================
--- ZODB/trunk/src/ZODB/MappingStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/MappingStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -274,7 +274,8 @@
     def tpc_begin(self, transaction, tid=None):
         # The tid argument exists to support testing.
         if transaction is self._transaction:
-            return
+            raise ZODB.POSException.StorageTransactionError(
+                "Duplicate tpc_begin calls for same transaction")
         self._lock_release()
         self._commit_lock.acquire()
         self._lock_acquire()
@@ -292,7 +293,8 @@
     @ZODB.utils.locked(opened)
     def tpc_finish(self, transaction, func = lambda tid: None):
         if (transaction is not self._transaction):
-            return
+            raise ZODB.POSException.StorageTransactionError(
+                "tpc_finish called with wrong transaction")
 
         tid = self._tid
         func(tid)
@@ -318,7 +320,9 @@
 
     # ZODB.interfaces.IStorage
     def tpc_vote(self, transaction):
-        pass
+        if transaction is not self._transaction:
+            raise POSException.StorageTransactionError(
+                "tpc_vote called with wrong transaction")
 
 class TransactionRecord:
 

Modified: ZODB/trunk/src/ZODB/interfaces.py
===================================================================
--- ZODB/trunk/src/ZODB/interfaces.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/interfaces.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -690,7 +690,7 @@
         """Begin the two-phase commit process.
 
         If storage is already participating in a two-phase commit
-        using the same transaction, the call is ignored.
+        using the same transaction, a StorageTransactionError is raised.
 
         If the storage is already participating in a two-phase commit
         using a different transaction, the call blocks until the
@@ -702,9 +702,10 @@
 
         Changes must be made permanent at this point.
 
-        This call is ignored if the storage isn't participating in
-        two-phase commit or if it is committing a different
-        transaction.  Failure of this method is extremely serious.
+        This call raises a StorageTransactionError if the storage
+        isn't participating in two-phase commit or if it is committing
+        a different transaction.  Failure of this method is extremely
+        serious.
 
         The second argument is a call-back function that must be
         called while the storage transaction lock is held.  It takes
@@ -715,9 +716,9 @@
     def tpc_vote(transaction):
         """Provide a storage with an opportunity to veto a transaction
 
-        This call is ignored if the storage isn't participating in
-        two-phase commit or if it is commiting a different
-        transaction.  Failure of this method is extremely serious.
+        This call raises a StorageTransactionError if the storage
+        isn't participating in two-phase commit or if it is commiting
+        a different transaction.
 
         If a transaction can be committed by a storage, then the
         method should return.  If a transaction cannot be committed,

Modified: ZODB/trunk/src/ZODB/tests/BasicStorage.py
===================================================================
--- ZODB/trunk/src/ZODB/tests/BasicStorage.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/tests/BasicStorage.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -34,8 +34,8 @@
     def checkBasics(self):
         t = transaction.Transaction()
         self._storage.tpc_begin(t)
-        # This should simply return
-        self._storage.tpc_begin(t)
+        self.assertRaises(POSException.StorageTransactionError,
+                          self._storage.tpc_begin, t)
         # Aborting is easy
         self._storage.tpc_abort(t)
         # Test a few expected exceptions when we're doing operations giving a

Modified: ZODB/trunk/src/ZODB/tests/Synchronization.py
===================================================================
--- ZODB/trunk/src/ZODB/tests/Synchronization.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/tests/Synchronization.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -99,19 +99,20 @@
 
     def checkFinishNotCommitting(self):
         t = Transaction()
-        self._storage.tpc_finish(t)
+        self.assertRaises(StorageTransactionError,
+                          self._storage.tpc_finish, t)
         self._storage.tpc_abort(t)
 
     def checkFinishWrongTrans(self):
         t = Transaction()
         self._storage.tpc_begin(t)
-        self._storage.tpc_finish(Transaction())
+        self.assertRaises(StorageTransactionError,
+                          self._storage.tpc_finish, Transaction())
         self._storage.tpc_abort(t)
 
     def checkBeginCommitting(self):
         t = Transaction()
         self._storage.tpc_begin(t)
-        self._storage.tpc_begin(t)
         self._storage.tpc_abort(t)
 
     # TODO:  how to check undo?

Modified: ZODB/trunk/src/ZODB/tests/testZODB.py
===================================================================
--- ZODB/trunk/src/ZODB/tests/testZODB.py	2010-01-08 22:12:50 UTC (rev 107888)
+++ ZODB/trunk/src/ZODB/tests/testZODB.py	2010-01-08 22:32:35 UTC (rev 107889)
@@ -420,8 +420,8 @@
             # performed yet.
             transaction.begin()
             log = self._db.undoLog()
-            for i in range(5):
-                self._db.undo(log[i]['id'])
+            self._db.undo([log[i]['id'] for i in range(5)])
+
             transaction.get().note('undo states 1 through 5')
 
             # Now attempt all those undo operations.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.