From 5b419041f69fa77af019f42278ee0fa4dac9c8c0 Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Mon, 29 Jun 2015 16:33:42 -0700 Subject: [PATCH 1/7] Add some tests for various transaction life-cycles --- repoze/sendmail/tests/test_transaction.py | 98 +++++++++++++++++++++-- 1 file changed, 93 insertions(+), 5 deletions(-) diff --git a/repoze/sendmail/tests/test_transaction.py b/repoze/sendmail/tests/test_transaction.py index 12646b9..31446cc 100644 --- a/repoze/sendmail/tests/test_transaction.py +++ b/repoze/sendmail/tests/test_transaction.py @@ -1,3 +1,4 @@ +import logging import unittest from repoze.sendmail.delivery import DirectMailDelivery @@ -7,6 +8,10 @@ class TestTransactionMails(unittest.TestCase): + def setUp(self): + import transaction + transaction.begin() + def test_abort(self): import transaction mailer = _makeMailerStub() @@ -101,7 +106,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.) + import transaction + 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. + import transaction + 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 +162,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) From 050815f1013c463a2657a56015ea8f4700f374db Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Mon, 29 Jun 2015 16:51:11 -0700 Subject: [PATCH 2/7] Make MailDataManager.abort less picky about when it gets called --- repoze/sendmail/delivery.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/repoze/sendmail/delivery.py b/repoze/sendmail/delivery.py index 59115d7..ac89f8d 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,10 +116,7 @@ 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)) @@ -168,11 +166,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) From 51b4aedd35d793f6f615cbecd4793ca4e25a51f9 Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Mon, 29 Jun 2015 17:00:12 -0700 Subject: [PATCH 3/7] Fix incorrect (now failing) tests --- repoze/sendmail/tests/test_delivery.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/repoze/sendmail/tests/test_delivery.py b/repoze/sendmail/tests/test_delivery.py index 9776943..77ccb82 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) @@ -265,10 +265,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 From 357b541e00fd0ae6443007f24653e288a4637005 Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Tue, 30 Jun 2015 06:56:54 -0700 Subject: [PATCH 4/7] Cleanup: Just import transaction once --- repoze/sendmail/tests/test_transaction.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/repoze/sendmail/tests/test_transaction.py b/repoze/sendmail/tests/test_transaction.py index 31446cc..a0afd07 100644 --- a/repoze/sendmail/tests/test_transaction.py +++ b/repoze/sendmail/tests/test_transaction.py @@ -2,6 +2,7 @@ import unittest from repoze.sendmail.delivery import DirectMailDelivery +import transaction from email.message import Message @@ -9,11 +10,9 @@ class TestTransactionMails(unittest.TestCase): def setUp(self): - import transaction transaction.begin() def test_abort(self): - import transaction mailer = _makeMailerStub() delivery = DirectMailDelivery(mailer) @@ -28,7 +27,6 @@ def test_abort(self): def test_doom(self): - import transaction mailer = _makeMailerStub() delivery = DirectMailDelivery(mailer) @@ -45,8 +43,6 @@ def test_doom(self): def test_savepoint(self): - import transaction - mailer = _makeMailerStub() delivery = DirectMailDelivery(mailer) ( fromaddr , toaddrs ) = fromaddr_toaddrs() @@ -110,7 +106,7 @@ def test_savepoint(self): 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.) - import transaction + mailer = _makeMailerStub(_failing=True) delivery = DirectMailDelivery(mailer) fromaddr, toaddrs = fromaddr_toaddrs() @@ -127,7 +123,7 @@ 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. - import transaction + mailer = _makeMailerStub() delivery = DirectMailDelivery(mailer) fromaddr, toaddrs = fromaddr_toaddrs() From a906da61c4f94fc02c8831b81be7ace151000834 Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Tue, 30 Jun 2015 08:00:05 -0700 Subject: [PATCH 5/7] Update CHANGES --- CHANGES.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index b6f44e7..e50a85b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,6 +6,12 @@ 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.) + 4.2 (2014-02-17) ---------------- From 74dac01d50c64fd5053a2bb1f01dfb87ad3a918c Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Tue, 30 Jun 2015 06:58:08 -0700 Subject: [PATCH 6/7] Sign CONTRIBUTORS.txt --- CONTRIBUTORS.txt | 2 ++ 1 file changed, 2 insertions(+) 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 From 72250406dd3fc6bf3e25572e60545c3286614971 Mon Sep 17 00:00:00 2001 From: Jeff Dairiki Date: Tue, 30 Jun 2015 07:22:28 -0700 Subject: [PATCH 7/7] Avoid raising execeptions during tpc_finish Commit (send) email from tpc_vote rather than tpc_finish. The documentation for ``transaction`` says that tpc_finish should never fail. Data managers which really have only "one-phase-commit" semantics (and whose commit operation may fail) should do the deed in tpc_vote. (At least, that's what zope.sqlalchemy does.) Refs: https://zodb.readthedocs.org/en/latest/transactions.html#tpc-finish --- CHANGES.rst | 7 +++++++ repoze/sendmail/delivery.py | 7 +++++-- repoze/sendmail/tests/test_delivery.py | 13 ++++++++++--- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index e50a85b..c5f8af6 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -12,6 +12,13 @@ Unreleased 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/repoze/sendmail/delivery.py b/repoze/sendmail/delivery.py index ac89f8d..5a39844 100644 --- a/repoze/sendmail/delivery.py +++ b/repoze/sendmail/delivery.py @@ -119,7 +119,10 @@ def abort(self, trans): 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 @@ -150,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: @@ -158,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): diff --git a/repoze/sendmail/tests/test_delivery.py b/repoze/sendmail/tests/test_delivery.py index 77ccb82..52476a9 100644 --- a/repoze/sendmail/tests/test_delivery.py +++ b/repoze/sendmail/tests/test_delivery.py @@ -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):