Make MailDataManager more lenient about when abort may be called

CHANGES.rst

@@ -6,6 +6,19 @@ Unreleased
 
 - Add support for Python 3.4, PyPy3.
 
+- ``MailDataManager.abort`` and ``MailDataManager.tpc_abort`` are now
+  more lenient regarding when they may be called in the transaction
+  life-cycle.  They were raising exceptions in some normal
+  failed-commit situations (masking the original exception
+  responsible for the failed transaction.)
+
+- ``MailDataManger`` now sends mail from ``tpc_vote`` rather than
+  ``tpc_finish``.  According to the `transaction documentation`_,
+  ``tpc_finish`` should never fail.
+
+.. _transaction documentation:
+   https://zodb.readthedocs.org/en/latest/transactions.html#tpc-finish
+
 4.2 (2014-02-17)
 ----------------
 

CONTRIBUTORS.txt

@@ -112,3 +112,5 @@ Contributors
 - Jonathan Vanasco, 2013/03/19
 
 - Patrick Strawderman, 2013/11/15
+
+- Geoffrey T. Dairiki, 2015/06/30

repoze/sendmail/delivery.py

@@ -101,6 +101,7 @@ def join_transaction(self, trans=None):
     def _finish(self, final_state):
         if self.transaction is None:
             raise ValueError("Not in a transaction")
+        self.tpc_phase = 3
         self.state = final_state
 
     def commit(self, trans):
@@ -115,13 +116,13 @@ def abort(self, trans):
             raise ValueError("Not in a transaction")
         if self.transaction is not trans:
             raise ValueError("In a different transaction")
-        if self.tpc_phase != 0:
-            raise ValueError("TPC in progress")
-        if self.onAbort:
-            self.onAbort()
+        self._do_abort()
 
     def sortKey(self):
-        return str(id(self))
+        # Currently, we only support "one-phase-commit" semantics.
+        # Try to sort last, so as to give all other data manager a
+        # chance to vote before email is sent.
+        return "~~~repoze.sendmail:%d" % id(self)
 
     def savepoint(self):
         """Create a custom `MailDataSavepoint` object
@@ -152,6 +153,7 @@ def tpc_vote(self, trans):
         if self.tpc_phase != 1:
             raise ValueError("TPC phase error: %d" % self.tpc_phase)
         self.tpc_phase = 2
+        self.callable(*self.args)
 
     def tpc_finish(self, trans):
         if self.transaction is None:
@@ -160,19 +162,22 @@ def tpc_finish(self, trans):
             raise ValueError("In a different transaction")
         if self.tpc_phase != 2:
             raise ValueError("TPC phase error: %d" % self.tpc_phase)
-        self.callable(*self.args)
         self._finish(MailDataManagerState.TPC_FINISHED)
 
     def tpc_abort(self, trans):
         if self.transaction is None:
             raise ValueError("Not in a transaction")
         if self.transaction is not trans:
             raise ValueError("In a different transaction")
-        if self.tpc_phase == 0:
-            raise ValueError("TPC phase error: %d" % self.tpc_phase)
-        if self.state is MailDataManagerState.TPC_FINISHED:
-            raise ValueError("TPC already finished")
-        self._finish(MailDataManagerState.TPC_ABORTED)
+        self._do_abort()
+
+    def _do_abort(self):
+        if self.state is MailDataManagerState.INIT:
+            if self.onAbort:
+                self.onAbort()
+            self._finish(MailDataManagerState.TPC_ABORTED)
+        elif self.state is not MailDataManagerState.TPC_ABORTED:
+            raise ValueError("TPC already finished: state=%d", self.state)
 
 
 @implementer(IDataManagerSavepoint)

repoze/sendmail/tests/test_delivery.py

@@ -122,7 +122,7 @@ def test_abort_w_TPC(self):
         txn = DummyTransaction()
         mdm.join_transaction(txn)
         mdm.tpc_phase = 1
-        self.assertRaises(ValueError, mdm.abort, txn)
+        mdm.abort(txn) # no raise
 
     def test_abort_w_same_transaction(self):
         mdm = self._makeOne(object)
@@ -142,7 +142,10 @@ def _onAbort():
 
     def test_sortKey(self):
         mdm = self._makeOne()
-        self.assertEqual(mdm.sortKey(), str(id(mdm)))
+        sort_key = mdm.sortKey()
+        self.assertTrue(sort_key > 'z')
+        self.assertTrue(sort_key > '42')
+        self.assertTrue(sort_key > '~sqlalchemy:42')
 
     def test_savepoint_wo_transaction(self):
         mdm = self._makeOne()
@@ -207,12 +210,16 @@ def test_tpc_vote_not_already_tpc(self):
         self.assertRaises(ValueError, mdm.tpc_vote, txn)
 
     def test_tpc_vote_ok(self):
-        mdm = self._makeOne()
+        _called = []
+        def _callable(*args):
+            _called.append(args)
+        mdm = self._makeOne(_callable, (1, 2))
         txn = DummyTransaction()
         mdm.join_transaction(txn)
         mdm.tpc_phase = 1
         mdm.tpc_vote(txn)
         self.assertEqual(mdm.tpc_phase, 2)
+        self.assertEqual(_called, [(1, 2)])
 
     def test_tpc_finish_wo_transaction(self):
         mdm = self._makeOne()
@@ -249,7 +256,7 @@ def _callable(*args):
         mdm.join_transaction(txn)
         mdm.tpc_phase = 2
         mdm.tpc_finish(txn)
-        self.assertEqual(_called, [(1, 2)])
+        self.assertEqual(_called, [])
         self.assertEqual(mdm.state, MailDataManagerState.TPC_FINISHED)
 
     def test_tpc_abort_wo_transaction(self):
@@ -265,10 +272,12 @@ def test_tpc_abort_w_foreign_transaction(self):
         self.assertRaises(ValueError, mdm.tpc_abort, txn2)
 
     def test_tpc_abort_not_already_tpc(self):
+        from ..delivery import MailDataManagerState
         mdm = self._makeOne()
         txn = DummyTransaction()
         mdm.join_transaction(txn)
-        self.assertRaises(ValueError, mdm.tpc_abort, txn)
+        mdm.tpc_abort(txn)
+        self.assertEqual(mdm.state, MailDataManagerState.TPC_ABORTED)
 
     def test_tpc_abort_already_finished(self):
         from ..delivery import MailDataManagerState

repoze/sendmail/tests/test_transaction.py

@@ -1,14 +1,18 @@
+import logging
 import unittest
 
 from repoze.sendmail.delivery import DirectMailDelivery
+import transaction
 from email.message import Message
 
 
 
 class TestTransactionMails(unittest.TestCase):
 
+    def setUp(self):
+        transaction.begin()
+
     def test_abort(self):
-        import transaction
         mailer = _makeMailerStub()
         delivery = DirectMailDelivery(mailer)
 
@@ -23,7 +27,6 @@ def test_abort(self):
 
 
     def test_doom(self):
-        import transaction
         mailer = _makeMailerStub()
         delivery = DirectMailDelivery(mailer)
 
@@ -40,8 +43,6 @@ def test_doom(self):
 
 
     def test_savepoint(self):
-        import transaction
-
         mailer = _makeMailerStub()
         delivery = DirectMailDelivery(mailer)
         ( fromaddr , toaddrs ) = fromaddr_toaddrs()
@@ -101,7 +102,44 @@ def test_savepoint(self):
             if i in bodies_bad :
                 sp.rollback()
         sp_outer.rollback()
-
+
+    def test_abort_after_failed_commit(self):
+        # It should be okay to call transaction.abort() after a failed
+        # commit.  (E.g. pyramid_tm does this.)
+
+        mailer = _makeMailerStub(_failing=True)
+        delivery = DirectMailDelivery(mailer)
+        fromaddr, toaddrs = fromaddr_toaddrs()
+        message = sample_message()
+        delivery.send(fromaddr, toaddrs, message)
+
+        with DummyLogHandler():  # Avoid "no handler could be found" on stderr
+            self.assertRaises(SendFailed, transaction.commit)
+
+        # An abort after commit failure should not raise exceptions
+        transaction.abort()
+
+    def test_commit_fails_before_tpc_vote(self):
+        # If there is a failure during transaction.commit() before all
+        # data managers have voted, .abort() is called on the non-voted
+        # managers before their .tpc_finish() is called.
+
+        mailer = _makeMailerStub()
+        delivery = DirectMailDelivery(mailer)
+        fromaddr, toaddrs = fromaddr_toaddrs()
+        message = sample_message()
+        delivery.send(fromaddr, toaddrs, message)
+
+        # Add another data manager whose tpc_vote fails before our
+        # delivery's tpc_vote gets called.
+        failing_dm = FailingDataManager(sort_key='!!! run first')
+        transaction.get().join(failing_dm)
+
+        try:
+            self.assertRaises(VoteFailure, transaction.commit)
+            self.assertEqual(mailer.sent_messages, [])
+        finally:
+            transaction.abort()
 
 
 def sample_message( body="This is just an example"):
@@ -120,19 +158,65 @@ def fromaddr_toaddrs():
     toaddrs = ('Guido <guido@example.com>',
                'Steve <steve@examplecom>')
     return ( fromaddr , toaddrs )
-
-
-
+
+
+class SendFailed(Exception):
+    pass
+
+
 def _makeMailerStub(*args, **kw):
     from zope.interface import implementer
     from repoze.sendmail.interfaces import IMailer
-    implementer(IMailer)
 
+    @implementer(IMailer)
     class MailerStub(object):
         def __init__(self, *args, **kw):
+            self.failing = kw.get('_failing', False)
             self.sent_messages = []
 
         def send(self, fromaddr, toaddrs, message):
+            if self.failing:
+                raise SendFailed("send failed")
             self.sent_messages.append((fromaddr, toaddrs, message))
+
     return MailerStub(*args, **kw)
 
+
+class VoteFailure(Exception):
+    pass
+
+
+class FailingDataManager(object):
+    def __init__(self, sort_key):
+        self.sort_key = sort_key
+
+    def sortKey(self):
+        return self.sort_key
+
+    def abort(self, trans):
+        pass
+
+    def tpc_begin(self, trans):
+        pass
+
+    def commit(self, trans):
+        pass
+
+    def tpc_vote(self, trans):
+        raise VoteFailure("vote failed")
+
+    def tpc_abort(self, trans):
+        pass
+
+
+class DummyLogHandler(logging.Handler):
+    def emit(self, record):
+        pass
+
+    def __enter__(self):
+        root_logger = logging.getLogger()
+        root_logger.addHandler(self)
+
+    def __exit__(self, typ, value, tb):
+        root_logger = logging.getLogger()
+        root_logger.removeHandler(self)