MailDataManager breaks the transaction manager upon abort

[mmerickel]

I have some code sending emails and for legitimate reasons the email is rejected by the remote server. I want to log the exception but instead I see the ValueError('TPC in progress') a couple times in the output, squashing the error.

It's actually causing the transaction manager to be in such a broken state that the next transaction.manager.begin() after the failed abort, causes another exception. Below is the crap fix to prove the diagnosis, but I'm not positive it's the correct fix. I downgraded to 4.1 and things work as expected.

From what I can tell this behavior appeared in 06ad882 when the manager was rewritten.

On a related side note, aborting in the second phase of a two-phase commit is a Really Bad Thing(tm). Since sending email isn't transactional it almost makes more sense to either send the email in an earlier phase or even as an afterCommitHook.

diff --git a/repoze/sendmail/delivery.py b/repoze/sendmail/delivery.py
index 59115d7..6b56add 100644
--- a/repoze/sendmail/delivery.py
+++ b/repoze/sendmail/delivery.py
@@ -115,8 +115,8 @@ class MailDataManager(object):
             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.tpc_phase != 0:
+        #    raise ValueError("TPC in progress")
         if self.onAbort:
             self.onAbort()

[mmerickel]

@jvanasco it'd be helpful if you could look at this since it appears to have been introduced by #24, #28

[jvanasco]

i'll take a look tonight and try to recreate the error.

sorry this happened. i lifted most of the code from the zope sqlaclhemy extension. it's had some updates, so I may be able to find some answers there.

[mcdonc]

I'm tempted to revert the changes in 4.2 and release a 4.3 that is actually just 4.1, as this issue is pretty serious.

[jvanasco]

Chris- I think that's a good idea.

• Savepoints in email isn't something many people needed.
• There were a handful of changes made to my original patch , so I'm not familiar enough with this
• I've been having too much trouble re-creating the error as a test-case without a post-commit hook

Michael-

I agree on no abort in phase 2.

On May 26, 2014, at 5:23 AM, Chris McDonough notifications@github.com wrote:

I'm tempted to revert the changes in 4.2 and release a 4.3 that is actually just 4.1, as this issue is pretty serious.

—
Reply to this email directly or view it on GitHub.

[jvanasco]

quick note, nowhere near a solution, but i've been making some progress on this in my fork [https://github.com/jvanasco/repoze.sendmail]

readability

• replaced all "phase/state numbers" with class variables.

• migrated some repetitive tasks into shared methods

• redid many of the new unit tests; they were mangling the 'state', but not actually going through the motions of actually doing the tpc_vote.

• added some debug logging

• starting to split out MailDataManager into two classes -- MailDataManager and TwoPhaseMailDataManager -- which is what Zope.SqlAlchemy has.

  i should be able to build a few more test cases for a the different abort issues, and then start to mirror the internals after ZopeSqlachemy.