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 [email protected] 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.

[ctheune]

Interesting. I'm stumbling over a "domain not found" and I really don't care. Downgrading to 4.1 doesn't fix it. This ends up in half-deliveries :(

[ctheune]

From what I can see the most reasonable part would be to bare-except the send() method completely: in the special case of email, I don't see why uncontrollably failing the whole transaction is better than to best-effort this. This is email anyway. We could do better if the data manager would have more control over the whole process and move some of the communication to the first phase (like rcpt, or other stuff).

[vietdt]

After struggling to debug this issue (and also related issue #30), I think the best and simplest approach is to reset the "tpc_phase" value to 0 after a two-phase commit completely finished or aborted. It then will not fail any later transaction.abort() calls. So both issues would be resolved.

I've implemented my fix in a fork vietdt@aba1cc5. Not sure if it works correctly and could be merged upstream.

diff --git a/repoze/sendmail/delivery.py b/repoze/sendmail/delivery.py
index 59115d7..28b7cf4 100644
--- a/repoze/sendmail/delivery.py
+++ b/repoze/sendmail/delivery.py
@@ -102,6 +102,7 @@ class MailDataManager(object):
         if self.transaction is None:
             raise ValueError("Not in a transaction")
         self.state = final_state
+        self.tpc_phase = 0

     def commit(self, trans):
         if self.transaction is None:

Also found the same implementation in a test script in transaction package
https://github.com/zopefoundation/transaction/blob/master/transaction/tests/savepointsample.py#L95-L98

[tseaver]

@vietdt Your suggested fix looks correct to me. @mmerickel can you test it against your failing case?

[vietdt]

@tseaver it seems there's no response from @mmerickel. Could anyone help on testing to confirm the fix? If the fix is correct, I would like to use it from mainstream package rather than from a fork or have to pin to version 4.1.

[papachoco]

Will there be a 4.3 release w/ the fix for this issue?