As discussed on zodb-dev, failing commit "sticks" now.

After a commit fails (raises an exception), all subsequent attempts
to commit, join, or register with the transaction now raise the new
TransactionFailedError.  The failed transaction must be explicitly
discarded now, via abort() on the transaction or begin() on its
transaction manager.

NEWS.txt

@@ -7,7 +7,7 @@ Connection
 
 ZODB intends to raise ConnnectionStateError if an attempt is made to close
 a connection while modifications are pending (the connection is involved in
-a transaction that hasn't been ``abort()``'ed or ``commit()``'ed). It was
+a transaction that hasn't been ``abort()``'ed or ``commit()``'ed).  It was
 missing the case where the only pending modifications were made in
 subtransactions.  This has been fixed.  If an attempt to close a connection
 with pending subtransactions is made now::
@@ -19,6 +19,39 @@ is raised.
 transaction
 -----------
 
+- Transactions have new, backward-incompatible behavior in one respect:
+  if a ``Transaction.commit()``, ``Transaction.commit(False)``, or
+  ``Transaction.commit(True)`` raised an exception, prior behavior was that
+  the transaction effectively aborted, and a new transaction began.
+  A primary bad consequence was that, if in a sequence of subtransaction
+  commits, one of the commits failed but the exception was suppressed,
+  all changes up to and including the failing commit were lost, but
+  later subtransaction commits in the sequence got no indication that
+  something had gone wrong, nor did the final (top level) commit.  This
+  could easily lead to inconsistent data being committed, from the
+  application's point of view.
+
+  The new behavior is that a failing commit "sticks" until explicitly
+  cleared.  Now if an exception is raised by a ``commit()`` call (whether
+  subtransaction or top level) on a Transaction object ``T``:
+
+    - Pending changes are aborted, exactly as they were for a failing
+      commit before.
+
+    - But ``T`` remains the current transaction object (if ``tm`` is ``T``'s
+      transaction manger, ``tm.get()`` continues to return ``T``).
+
+    - All subsequent attempts to do ``T.commit()``, ``T.join()``, or
+      ``T.register()`` raise the new ``TransactionFailedError`` exception.
+      Note that if you try to modify a persistent object, that object's
+      resource manager (usually a ``Connection`` object) will attempt to
+      ``join()`` the failed transaction, and ``TransactionFailedError``
+      will be raised right away.
+
+  So after a transaction or subtransaction commit fails, that must be
+  explicitly cleared now, either by invoking ``abort()`` on the transaction
+  object, or by invoking ``begin()`` on its transaction manager.
+
 - Some explanations of new transaction features in the 3.3a3 news
   were incorrect, and this news file has been retroactively edited to
   repair that.  See news for 3.3a3 below.
@@ -44,12 +77,13 @@ transaction
       >>> import transaction
       >>> txn = transaction.begin()  # start a txn using the default TM
 
-  if using the default ThreadTransactionManager (see news for 3.3a3 below).
-  In 3.3, it's intended that a single Transaction object is used for exactly
-  one transaction.  So, unlike as in 3.2, when somtimes Transaction objects
-  were reused across transactions, but sometimes weren't, when you do
-  ``Transaction.begin()`` in 3.3 a brand new transaction object is
-  created.  That's why this use is deprecated.  Code of the form:
+  if using the default ``ThreadTransactionManager`` (see news for 3.3a3
+  below). In 3.3, it's intended that a single ``Transaction`` object is
+  used for exactly one transaction.  So, unlike as in 3.2, when somtimes
+  ``Transaction`` objects were reused across transactions, but sometimes
+  weren't, when you do ``Transaction.begin()`` in 3.3 a brand new
+  transaction object is created.  That's why this use is deprecated.  Code
+  of the form:
 
       >>> txn = transaction.get()
       >>> ...
@@ -57,10 +91,8 @@ transaction
       >>> ...
       >>> txn.commit()
 
-  can't work as intended is 3.3, because ``txn`` is no longer the current
-  Transaction object the instant ``txn.begin()`` returns.
-
-
+  can't work as intended ib 3.3, because ``txn`` is no longer the current
+  ``Transaction`` object the instant ``txn.begin()`` returns.
 
 BTrees
 ------

src/ZODB/POSException.py

@@ -33,6 +33,16 @@ class POSKeyError(KeyError, POSError):
 class TransactionError(POSError):
     """An error occured due to normal transaction processing."""
 
+class TransactionFailedError(POSError):
+    """Cannot perform an operation on a transaction that previously failed.
+
+    An attempt was made to commit a transaction, or to join a transaction,
+    but this transaction previously raised an exception during an attempt
+    to commit it.  The transaction must be explicitly aborted, either by
+    invoking abort() on the transaction, or begin() on its transaction
+    manager.
+    """
+
 class ConflictError(TransactionError):
     """Two transactions tried to modify the same object at once.
 

src/ZODB/__init__.py

@@ -26,6 +26,6 @@ from transaction import get as get_transaction
 # really be in persistent anyway.
 sys.modules['ZODB.TimeStamp'] = sys.modules['persistent.TimeStamp']
 
-# XXX Issue deprecation warning if this variant is used?
+# TODO Issue deprecation warning if this variant is used?
 __builtin__.get_transaction = get_transaction
 del __builtin__

src/ZODB/tests/testZODB.py

@@ -16,6 +16,7 @@ import unittest
 import ZODB
 import ZODB.FileStorage
 from ZODB.POSException import ReadConflictError, ConflictError
+from ZODB.POSException import TransactionFailedError
 from ZODB.tests.warnhook import WarningsHook
 
 from persistent import Persistent
@@ -254,7 +255,7 @@ class ZODBTests(unittest.TestCase):
         self.obj = P()
         self.readConflict()
 
-    def readConflict(self, shouldFail=1):
+    def readConflict(self, shouldFail=True):
         # Two transactions run concurrently.  Each reads some object,
         # then one commits and the other tries to read an object
         # modified by the first.  This read should fail with a conflict
@@ -290,6 +291,15 @@ class ZODBTests(unittest.TestCase):
             # the transaction should re-raise it.  checkNotIndependent()
             # failed this part of the test for a long time.
             self.assertRaises(ReadConflictError, tm2.get().commit)
+
+            # And since that commit failed, trying to commit again should
+            # fail again.
+            self.assertRaises(TransactionFailedError, tm2.get().commit)
+            # And again.
+            self.assertRaises(TransactionFailedError, tm2.get().commit)
+            # Etc.
+            self.assertRaises(TransactionFailedError, tm2.get().commit)
+
         else:
             # make sure that accessing the object succeeds
             obj.child1
@@ -337,12 +347,13 @@ class ZODBTests(unittest.TestCase):
         self.assert_(not index2[0]._p_changed)
         self.assert_(not index2[1]._p_changed)
 
-        self.assertRaises(ConflictError, tm.get().commit)
-        transaction.abort()
+        self.assertRaises(ReadConflictError, tm.get().commit)
+        self.assertRaises(TransactionFailedError, tm.get().commit)
+        tm.get().abort()
 
     def checkIndependent(self):
         self.obj = Independent()
-        self.readConflict(shouldFail=0)
+        self.readConflict(shouldFail=False)
 
     def checkNotIndependent(self):
         self.obj = DecoyIndependent()
@@ -438,6 +449,149 @@ class ZODBTests(unittest.TestCase):
         finally:
             del warnings.filters[0]
 
+    def checkFailingCommitSticks(self):
+        # See also checkFailingSubtransactionCommitSticks.
+        cn = self._db.open()
+        rt = cn.root()
+        rt['a'] = 1
+
+        # Arrange for commit to fail during tpc_vote.
+        poisoned = PoisonedObject(PoisonedJar(break_tpc_vote=True))
+        transaction.get().register(poisoned)
+
+        self.assertRaises(PoisonedError, transaction.get().commit)
+        # Trying to commit again fails too.
+        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        self.assertRaises(TransactionFailedError, transaction.get().commit)
+
+        # The change to rt['a'] is lost.
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+        # Trying to modify an object also fails, because Transaction.join()
+        # also raises TransactionFailedError.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+
+        # Clean up via abort(), and try again.
+        transaction.get().abort()
+        rt['a'] = 1
+        transaction.get().commit()
+        self.assertEqual(rt['a'], 1)
+
+        # Cleaning up via begin() should also work.
+        rt['a'] = 2
+        transaction.get().register(poisoned)
+        self.assertRaises(PoisonedError, transaction.get().commit)
+        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        # The change to rt['a'] is lost.
+        self.assertEqual(rt['a'], 1)
+        # Trying to modify an object also fails.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+        # Clean up via begin(), and try again.
+        transaction.begin()
+        rt['a'] = 2
+        transaction.get().commit()
+        self.assertEqual(rt['a'], 2)
+
+        cn.close()
+
+    def checkFailingSubtransactionCommitSticks(self):
+        cn = self._db.open()
+        rt = cn.root()
+        rt['a'] = 1
+        transaction.get().commit(True)
+        self.assertEqual(rt['a'], 1)
+
+        rt['b'] = 2
+        # Subtransactions don't do tpc_vote, so we poison tpc_begin.
+        poisoned = PoisonedObject(PoisonedJar(break_tpc_begin=True))
+        transaction.get().register(poisoned)
+        self.assertRaises(PoisonedError, transaction.get().commit, True)
+        # Trying to subtxn-commit again fails too.
+        self.assertRaises(TransactionFailedError, transaction.get().commit, True)
+        self.assertRaises(TransactionFailedError, transaction.get().commit, True)
+        # Top-level commit also fails.
+        self.assertRaises(TransactionFailedError, transaction.get().commit)
+
+        # The changes to rt['a'] and rt['b'] are lost.
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+        self.assertRaises(KeyError, rt.__getitem__, 'b')
+
+        # Trying to modify an object also fails, because Transaction.join()
+        # also raises TransactionFailedError.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+
+        # Clean up via abort(), and try again.
+        transaction.get().abort()
+        rt['a'] = 1
+        transaction.get().commit()
+        self.assertEqual(rt['a'], 1)
+
+        # Cleaning up via begin() should also work.
+        rt['a'] = 2
+        transaction.get().register(poisoned)
+        self.assertRaises(PoisonedError, transaction.get().commit, True)
+        self.assertRaises(TransactionFailedError, transaction.get().commit, True)
+        # The change to rt['a'] is lost.
+        self.assertEqual(rt['a'], 1)
+        # Trying to modify an object also fails.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+        # Clean up via begin(), and try again.
+        transaction.begin()
+        rt['a'] = 2
+        transaction.get().commit(True)
+        self.assertEqual(rt['a'], 2)
+        transaction.get().commit()
+
+        cn2 = self._db.open()
+        rt = cn.root()
+        self.assertEqual(rt['a'], 2)
+
+        cn.close()
+        cn2.close()
+
+class PoisonedError(Exception):
+    pass
+
+# PoisonedJar arranges to raise exceptions from interesting places.
+# For whatever reason, subtransaction commits don't call tpc_vote.
+class PoisonedJar:
+    def __init__(self, break_tpc_begin=False, break_tpc_vote=False):
+        self.break_tpc_begin = break_tpc_begin
+        self.break_tpc_vote = break_tpc_vote
+
+    def sortKey(self):
+        return str(id(self))
+
+    # A way to poison a subtransaction commit.
+    def tpc_begin(self, *args):
+        if self.break_tpc_begin:
+            raise PoisonedError("tpc_begin fails")
+
+    # A way to poison a top-level commit.
+    def tpc_vote(self, *args):
+        if self.break_tpc_vote:
+            raise PoisonedError("tpc_vote fails")
+
+    # commit_sub is needed else this jar is ignored during subtransaction
+    # commit.
+    def commit_sub(*args):
+        pass
+
+    def abort_sub(*args):
+        pass
+
+    def commit(*args):
+        pass
+
+    def abort(*self):
+        pass
+
+
+class PoisonedObject:
+    def __init__(self, poisonedjar):
+        self._p_jar = poisonedjar
+
 def test_suite():
     return unittest.makeSuite(ZODBTests, 'check')
 

src/ZODB/tests/testmvcc.py

@@ -147,9 +147,10 @@ Traceback (most recent call last):
  ...
 ConflictError: database conflict error (oid 0x01, class ZODB.tests.MinPO.MinPO)
 
-The failed commit aborted the current transaction, so we can try
-again.  This example will demonstrate that we can commit a transaction
-if we only modify current revisions.
+>>> tm2.get().abort()
+
+This example will demonstrate that we can commit a transaction if we only
+modify current revisions.
 
 >>> print cn2._txn_time
 None

src/transaction/_transaction.py

@@ -137,6 +137,16 @@ import logging
 import sys
 import thread
 import warnings
+import traceback
+from cStringIO import StringIO
+
+# Sigh.  In the maze of __init__.py's, ZODB.__init__.py takes 'get'
+# out of transaction.__init__.py, in order to stuff the 'get_transaction'
+# alias in __builtin__.  So here in _transaction.py, we can't import
+# exceptions from ZODB.POSException at top level (we're imported by
+# our __init__.py, which is imported by ZODB's __init__, so the ZODB
+# package isn't well-formed when we're first imported).
+# from ZODB.POSException import TransactionError, TransactionFailedError
 
 _marker = object()
 
@@ -146,13 +156,15 @@ def myhasattr(obj, attr):
     return getattr(obj, attr, _marker) is not _marker
 
 class Status:
+    # ACTIVE is the initial state.
+    ACTIVE       = "Active"
+
+    COMMITTING   = "Committing"
+    COMMITTED    = "Committed"
 
-    ACTIVE = "Active"
-    COMMITTING = "Committing"
-    COMMITTED = "Committed"
-    ABORTING = "Aborting"
-    ABORTED = "Aborted"
-    FAILED = "Failed"
+    # commit() or commit(True) raised an exception.  All further attempts
+    # to commit or join this transaction will raise TransactionFailedError.
+    COMMITFAILED = "Commit failed"
 
 class Transaction(object):
 
@@ -186,8 +198,26 @@ class Transaction(object):
         # subtransactions that were involved in subtransaction commits.
         self._nonsub = {}
 
+        # If a commit fails, the traceback is saved in _failure_traceback.
+        # If another attempt is made to commit, TransactionFailedError is
+        # raised, incorporating this traceback.
+        self._failure_traceback = None
+
+    # Raise TransactionFailedError, due to commit()/join()/register()
+    # getting called when the current transaction has already suffered
+    # a commit failure.
+    def _prior_commit_failed(self):
+        from ZODB.POSException import TransactionFailedError
+        assert self._failure_traceback is not None
+        raise TransactionFailedError("commit() previously failed, "
+                "with this traceback:\n\n%s" %
+                self._failure_traceback.getvalue())
+
     def join(self, resource):
-        if self.status != Status.ACTIVE:
+        if self.status is Status.COMMITFAILED:
+            self._prior_commit_failed() # doesn't return
+
+        if self.status is not Status.ACTIVE:
             # TODO: Should it be possible to join a committing transaction?
             # I think some users want it.
             raise ValueError("expected txn status %r, but it's %r" % (
@@ -244,6 +274,9 @@ class Transaction(object):
         # transaction.
 
     def commit(self, subtransaction=False):
+        if self.status is Status.COMMITFAILED:
+            self._prior_commit_failed() # doesn't return
+
         if not subtransaction and self._sub and self._resources:
             # This commit is for a top-level transaction that has
             # previously committed subtransactions.  Do one last
@@ -256,7 +289,20 @@ class Transaction(object):
                 s.beforeCompletion(self)
             self.status = Status.COMMITTING
 
-        self._commitResources(subtransaction)
+        try:
+            self._commitResources(subtransaction)
+        except:
+            self.status = Status.COMMITFAILED
+            # Save the traceback for TransactionFailedError.
+            ft = self._failure_traceback = StringIO()
+            t, v, tb = sys.exc_info()
+            # Record how we got into commit().
+            traceback.print_stack(sys._getframe(1), None, ft)
+            # Append the stack entries from here down to the exception.
+            traceback.print_tb(tb, None, ft)
+            # Append the exception type and value.
+            ft.writelines(traceback.format_exception_only(t, v))
+            raise t, v, tb
 
         if subtransaction:
             self._resources = []
@@ -315,9 +361,6 @@ class Transaction(object):
                 self._cleanup(L)
             finally:
                 if not subtransaction:
-                    self.status = Status.FAILED
-                    if self._manager:
-                        self._manager.free(self)
                     for s in self._synchronizers:
                         s.afterCompletion(self)
             raise t, v, tb
@@ -389,6 +432,7 @@ class Transaction(object):
                 s.beforeCompletion(self)
 
         if subtransaction and self._nonsub:
+            from ZODB.POSException import TransactionError
             raise TransactionError("Resource manager does not support "
                                    "subtransaction abort")