diff --git a/CHANGES.rst b/CHANGES.rst index b6f44e7..c5f8af6 100644 --- a/CHANGES.rst +++ b/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) ---------------- diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 150def7..3764cbd 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -112,3 +112,5 @@ Contributors - Jonathan Vanasco, 2013/03/19 - Patrick Strawderman, 2013/11/15 + +- Geoffrey T. Dairiki, 2015/06/30 diff --git a/repoze/sendmail/delivery.py b/repoze/sendmail/delivery.py index 59115d7..5a39844 100644 --- a/repoze/sendmail/delivery.py +++ b/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,7 +162,6 @@ 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): @@ -168,11 +169,15 @@ def tpc_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 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) diff --git a/repoze/sendmail/tests/test_delivery.py b/repoze/sendmail/tests/test_delivery.py index 9776943..52476a9 100644 --- a/repoze/sendmail/tests/test_delivery.py +++ b/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 diff --git a/repoze/sendmail/tests/test_transaction.py b/repoze/sendmail/tests/test_transaction.py index 12646b9..a0afd07 100644 --- a/repoze/sendmail/tests/test_transaction.py +++ b/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 ', 'Steve ') 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)