transaction 3.0 breaks afterCommitHooks

[icemac]

What I did:

A customer has an application using ZODB and zope.sqlalchemy running on transaction 2.4.0. I wanted to upgrade it to 3.0.
In the afterCommitHook the relational database is accessed via zope.sqlalchemy, changes are made and committed.
This worked in 2.4 because in tpc_finish the database session is closed and self._free() is called before calling the after commit hooks. This way the after commit hook had a clean state to start a new transaction.

What actually happened:

transaction 3.0 moved the self._free() call after calling the after commit hooks, see c76fd72#diff-b36b9bfe40a935a0303d1998fd1e0b8c1a062ced96eb4f92858e3b73409352c7L315-R323.

Using zope.sqlalcemy in the after commit hook now requires it to join the transaction because in tpc_finish it closed its session and forgot the knowledge about the zope transaction. But joining a transaction in the status committed is prohibited.

zope.sqlalchemy always seems to try to join the zope transaction. If that fails, it already had created the internal SQLAlchemy transaction. So using try-except in this case creates problems later on when SQLAlchemy thinks it does not have to join the zope transaction because it already has is internal transaction object initialized.

Blindly starting a new transaction in the application code used in the after commit hook is no solution as it is also used by code running inside the transaction.

Questions

• Was the change of calling self._free() earlier intended to change a broken (?) behavior?
• Are there any suggestions how to act inside an after commit hook now, when having the need to commit something? (Was this ever supposed to be possible?)
• Is my issue a problem in transaction, zope.sqlalchemy, or in the application code?

What version of Python and Zope/Addons I am using:

• transaction 2.4 -> 3.0
• Python 3.7
• zope.sqlalchemy 1.3
• SQLAlchemy 1.3.20

I am going to assign this to @d-maurer as the author of the change in transaction 3.0 but maybe others have an opinion, too.

[d-maurer]

Michael Howitz wrote at 2020-11-6 01:33 -0800:

... ### Questions * Was the change of calling `self._free()` earlier intended to change a broken (?) behavior?

At the moment, I no longer know why I have moved the `_free` call. I will look into this (likely early next week) and hope to find out again.

* Are there any suggestions how to act inside an after commit hook now, when having the need to commit something? (Was this ever supposed to be possible?)

The use case surprises me: as the actions are performed inside a transaction commit, they seem to be tightly coupled to the transaction. If they modify persistent data, should they not be part of the (committed) transaction itself? Otherwise, it is possible that the transaction is persisted but the effect of the post commit action is lost (e.g. when its local transaction commit fails). I have never imagined that one would want to do something like this. `transaction` has the additional concept `synchronizer`. I have the feeling that their coupling to the transaction is a bit weaker than that of post commit hooks -- it might be possible to start a new transaction in the synchronizer call. Maybe, this is conceptionally better suited for `zope.sqlalchemy`.

* Is my issue a problem in transaction, zope.sqlalchemy, or in the application code?

It is not obvious when the transaction under commit should end (i.e. to be `_free`ed), making it possible to start the next transaction. It is clear that it cannot happen before the `tpc_finish` and must happen before the `commit` returns; in between there is room for discussion. As outlined above, I find it strange to start a new transaction in an after commit hook: this transaction can obviously fail while the original transaction succeeded. "zopefoundation/ZODB3#2" and (related) "plone/collective.indexing#8" indicate that some restrictions during the `commit` call are to be expected.

[d-maurer]

Michael Howitz wrote at 2020-11-6 01:33 -0800:

... * Was the change of calling `self._free()` earlier intended to change a broken (?) behavior?

The motivation is documented in comment "#81 (comment)"

[d-maurer]

Michael Howitz wrote at 2020-11-6 01:33 -0800:

... * Are there any suggestions how to act inside an after commit hook now, when having the need to commit something? (Was this ever supposed to be possible?)

`_free` is idempotent (followup calls are effective no-ops). This allows a hook to call `transaction._free` itself. Calling `_free` ends the transaction and (usually) gets a new one implicitly started. Note that `_free` reinitializes the transaction, especially (without special measures) further hooks will not run. The preceding note also means that we cannot simply move the `_free` call back before the `call_hooks`. If called at this place, no hooks would be executed. That `_free` clears all hooks was newly introduced in "#81 (comment)" to avoid potential memory cycles.

[icemac]

So it seems that the postCommitHooks should not do anything what has to be transactional or triggers the transaction machinery. Let's see what I can do with this knowledge in the application of the customer.

[icemac]

@d-maurer Thank you for looking into this and answering my questions.

Id like to keep this issue open for a while und I found a solution together with the customer.

[d-maurer]

Michael Howitz wrote at 2020-11-11 07:16 -0800:

So it seems that the postCommitHooks should not do anything what has to be transactional or triggers the transaction machinery. Let's see what I can do with this knowledge in the application of the customer.

There is an additional possibility for your concrete case: `Transaction._free` has two parts: 1. `_free_manager`: this actually tells the transaction manager that the current transaction is about to end. The default transaction manager would then set up a new (current) transaction 2. cleanup for the transaction itself: deleting `_data`, cleaning `resources` and `hooks`. Your hook could call `_free_manager` to allow a new transaction to be started. Like `_free`, `_free_manager` is idempotent: there is no risk to call it several times.

[icemac]

@d-maurer Thank you for your suggestion it helped for one problem but broke other tests with different strange error messages. So the recommendation would be to move the code outside the after commit hook and run it asynchronously.

As no-one else seems to have this problem, I am closing the issue.