search

zopefoundation / transaction

Generic transaction implementation for Python.
Other
70 stars 31 forks source link

transaction 3.0 breaks afterCommitHooks #98

Closed icemac closed 3 years ago

icemac commented 4 years ago

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 https://github.com/zopefoundation/transaction/commit/c76fd72845374fe97dbc195c9cb8489933ce4b84#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

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

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 commented 4 years ago

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 _freeed), 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.

"https://github.com/zopefoundation/ZODB3/issues/2" and (related) "https://github.com/plone/collective.indexing/issues/8" indicate that some restrictions during the commit call are to be expected.

d-maurer commented 4 years ago

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 "https://github.com/zopefoundation/transaction/pull/81#issuecomment-492535572"

d-maurer commented 4 years ago

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 "https://github.com/zopefoundation/transaction/pull/81#issuecomment-492335257" to avoid potential memory cycles.

icemac commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 3 years ago

@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.