MailDataManager raises exception when transaction has after-commit hooks

[kilink]

If a transaction has any after-commit hooks, abort is called on the transaction's resources after the hooks are called (see the _callAfterCommitHooks method):

https://github.com/zopefoundation/transaction/blob/master/transaction/_transaction.py#L372-L378

The MailDataManager treats this as an error condition and raises an exception without calling its onAbort callback.

I've added a test demonstrating the issue: kilink/repoze.sendmail@c26361e7ac4

(Note that I add the log handler just to demonstrate that an exception is being raised.)

I think the best way to deal with this may be to increment the MailDataManager's tpc_phase attribute in tpc_finish, and then check if the two-phase commit finished in the abort method; if that's the case, no exception should be raised.

Another question is whether the onAbort callback should be called in this case. I'm not sure that it should.

[kilink]

@tseaver, do you have any opinions on this? The issue is more annoying than harmful (if you monitor exceptions or logs), but it would be nice to fix.

[tseaver]

@kilink I don't really have much of an opinion. I think I would likely make MDM try to behave like a ZODB.Connection as much as possible.

[mmerickel]

I opened #31 instead of posting my issue here. It looks related though.

[tseaver]

@kilink the fix suggested by @vietdt in #31 looks like it would resolve your issue: can you verify that it does?

[kilink]

It's been a couple years since I've used this package, but I tried applying the suggested fix just for curiosity's sake, and it does resolve the first issue where the MailDataManager would raise an exception.

However, with this fix the issue of whether or not to call MailDataManager.onAbort must be addressed. The Maildir implementation is hostile to calling abort after commit, so an exception will still be raised when after-commit hooks are involved.

Another thing worth noting is that I couldn't reproduce the problem with transaction >= 1.6.0, because of changes made in 2eb00f90. The transaction's resources get cleared now after a successful commit, but before the after-commit hooks are called, so MailDataManger is never aborted in this case.

Somewhat of a tangent, but the offending code therefore only gets called when the transaction does not succeed; however, the resources are already all aborted at that point via a previous call to the _cleanup method, so maybe this code can be removed altogether.

[jamadden]

This is still a problem with the most recent version of transaction and master from here, at least with MaildirTransactionalMessage and QueuedMailDelivery.

# test.py
from email.message import Message

import transaction
from repoze.sendmail import delivery


qmd = delivery.QueuedMailDelivery('/tmp/maildir_q')
msg = Message()

transaction.get().addAfterCommitHook(
    lambda *args: print("After Commit:", *args))

qmd.send('[email protected]', ['[email protected]'], msg)

transaction.commit()

$ python /tmp/g.py
After Commit: True
Error in abort() on manager <repoze.sendmail.delivery.MailDataManager object at 0x104b21d70>
Traceback (most recent call last):
  File "//python3.8/site-packages/transaction/_transaction.py", line 392, in _call_hooks
    rm.abort(self)
  File "//repoze.sendmail/repoze/sendmail/delivery.py", line 122, in abort
    self.onAbort()
  File "/repoze.sendmail/repoze/sendmail/maildir.py", line 133, in abort
    raise RuntimeError('Cannot abort--already committed.')
RuntimeError: Cannot abort--already committed.

PR #35 exists to solve this, but it's based on an older version of the code (and doesn't come close to cleanly applying today). Would a new PR that makes MaildirTransactionalMessage acknowledge (and ignore) that it can be called after commit be accepted? Something like this (before tests):

--------------------------------------
modified: repoze/sendmail/maildir.py
--------------------------------------
@@ -127,13 +127,9 @@ class MaildirTransactionalMessage(object):
         self._committed = True

     def abort(self):
-        if self._aborted:
-            return
-        if self._committed:
-            raise RuntimeError('Cannot abort--already committed.')
-
         self._aborted = True
-        os.remove(self._pending_path)
+        if os.path.exists(self._pending_path):
+            os.remove(self._pending_path)

     def __del__(self):
         if (not self._aborted and