Update events to play nice with transactions #2594

alcaeus · 2023-11-29T13:17:52Z

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

With transactional commit enabled, there's a chance that lifecycle events get dispatched more than once. This can be a problem for existing code that may assume that lifecycle events only get dispatched once, especially since this is also the behaviour when using transactions in ORM. Since a failing transaction may lead to retrying the entire commit operation, we need to make sure to only invoke lifecycle events once.

This is done by adding a transactional mode to the LifecycleEventManager class, which then tracks which events have been dispatched for each document. The preUpdate, postPersist, postUpdate, and postRemove events are then skipped if they have previously been dispatched for this document.

In addition to the above, the LifecycleEventArgs is now capable of telling event handlers whether the operation is part of a transaction. This and the session property in the event is necessary for any lifecycle event handlers that want to access the database. Since any changes already made to the database are only visible within the transaction, event handlers that want to read or write data need to pass the session option to ensure the write happens within the same transaction.

jmikola · 2023-11-29T15:36:24Z

lib/Doctrine/ODM/MongoDB/Event/LifecycleEventArgs.php

+        public readonly bool $isInTransaction = false,
+        public readonly ?Session $session = null,


Quick n00b question: why do we need bool flag while there's $session->isInTransaction() present? At a glance it seems we could add an isser return $session?->isInInTransaction() ?? false instead of a property?

Good point. I've added a new method and removed the $isInTransaction constructor parameter.

lib/Doctrine/ODM/MongoDB/Event/PreLoadEventArgs.php

lib/Doctrine/ODM/MongoDB/Event/PreUpdateEventArgs.php

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php

tests/Doctrine/ODM/MongoDB/Tests/Events/TransactionalLifecycleEventsTest.php

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php

malarzm · 2023-12-17T18:59:31Z

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php

+        if (! $this->shouldDispatchEvent($document, Events::postPersist)) {
+            return;
+        }
+


Should there be a failsafe checking for $session !== null when transactional mode is enabled? Or are we separating db-transaction from this concept?

I was planning on separating these concepts, unless there's a good reason to check whether the session is in a transaction. WDYT @jmikola?

Given that LifecycleEventManager is an internal class, I imagine $session should always be non-null if ODM is behaving properly.

Would this be an appropriate use case for an assert(), or perhaps an InvalidArgumentException would be more user-friendly? Either way, it seems reasonable to have some sanity check for this even though it's only relevant to how the event args are constructed.

See also: #2594 (comment)

jmikola

I have some thoughts on the Session assertion in LifecycleEventManager, but the rest of this LGTM.

jmikola · 2023-12-20T18:44:56Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -74,7 +74,14 @@
 *
 * @template T of object
 *
- * @psalm-import-type CommitOptions from UnitOfWork
+ * @psalm-type CommitOptions array{


Noted that this was introduced to resolve #2594 (comment).

Also, TIL about @psalm-import-type, which was originally introduced in e00e9e9#diff-ddc55123df78abdb7a5c057a6e80364bb4a234271e18aac03392e6939ed3b26aR48. I think that answers a previous question I had about re-using type definitions in PHPLIB.

jmikola · 2023-12-20T18:48:24Z

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php

+        if (! $this->shouldDispatchEvent($document, Events::postPersist)) {
+            return;
+        }
+


Given that LifecycleEventManager is an internal class, I imagine $session should always be non-null if ODM is behaving properly.

Would this be an appropriate use case for an assert(), or perhaps an InvalidArgumentException would be more user-friendly? Either way, it seems reasonable to have some sanity check for this even though it's only relevant to how the event args are constructed.

See also: #2594 (comment)

jmikola · 2023-12-20T18:54:49Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

@@ -456,6 +456,8 @@ public function commit(array $options = []): void
            $this->evm->dispatchEvent(Events::onFlush, new Event\OnFlushEventArgs($this->dm));

            if ($this->useTransaction($options)) {
+                $this->lifecycleEventManager->enableTransactionalMode();


Related to @malarzm's other thread asking about asserting that $session is non-null when LifecycleEventManager dispatches events, this appears to be the only place where transactional mode is enabled, and that is done based on the withTransaction option being specified (no direct Session check at this point).

I realize with_transaction() below is going to be responsible for passing a Session, which we'll then forward to doCommit(), but I don't believe there will be any further checks on a non-null Session after that point.

In that case, perhaps it's undesirable to perform such an assertion in LifecycleEventManager. But if we do add a check, I'd at least suggest capturing this logic in a comment for the first assertion within the file (to explain why we're asserting that the Session is set, and how we're trusting UnitOfWork to ferry it along to the dispatch method).

Good point. I added checks for this: enableTransactionalMode now requires a session that will be stored. When checking if an event should be dispatched, the given session is compared with the one stored to ensure we're still running in the same transaction. This should guard against dispatching events without a session in transactional mode.

* Run CI workflows on feature branches * Refactor commit logic (#2580) * Add tests for commit consistency showing wrong behaviour * Clear scheduled document changes at the end of a commit operation * Rename private variables to communicate intent * Remove obsolete comment * Use different error code * Use single mongos in consistency tests This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses. * Rename test methods for clarity * Explain reasoning for index error * Extract helper method to create failpoint * Add configuration setting for transactional flush (#2587) * Add configuration setting for transactional flush * Use classic setters for transactional flush setting * Add logic for transactional commits (#2589) * Extract commit logic * Support transactional commit operations * Always use transactional flush if supported With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour. * Test with MongoDB 7.0 * Update test names * Update phpstan baseline * Fix query selection in shard key tests * Flip transaction options constant by default * Use supportsTransaction method when skipping tests * Apply review feedback to tests * Add separate test to check write concern in commit options * Strip write options when in transaction * Use majority write concern in test * Update events to play nice with transactions (#2594) * Pass session and transaction information to event args * Only dispatch lifecycle events once per commit operation * Remove isInTransaction property in event args * Split method signature for readability * Use property promotion for event args classes * Extract construction of eventArgs * Inline spl_object_hash calls * Avoid injecting test instance * Add session to commitOptions in persister * Add session assertions in LifecycleEventManager * Only retry transaction once (#2604) * Only retry transaction once * Rename variable * Update transaction documentation (#2606) * Remove references to transactions where not applicable * Update transaction documentation * Apply suggestions from code review Co-authored-by: Jeremy Mikola <[email protected]> * Change title level in event documentation * Add documentation about transactions to event docs --------- Co-authored-by: Jeremy Mikola <[email protected]> * Address code review items * Test highest dependencies on MongoDB 7.0 * Regenerate psalm baseline to silence unrelated errors --------- Co-authored-by: Jeremy Mikola <[email protected]>

alcaeus requested review from jmikola and malarzm November 29, 2023 13:17

alcaeus self-assigned this Nov 29, 2023

jmikola reviewed Nov 29, 2023

View reviewed changes

malarzm reviewed Dec 17, 2023

View reviewed changes

lib/Doctrine/ODM/MongoDB/Utility/LifecycleEventManager.php Show resolved Hide resolved

malarzm reviewed Dec 17, 2023

View reviewed changes

alcaeus force-pushed the feature/transactions branch from 7c32c01 to 18a61c2 Compare December 19, 2023 10:48

alcaeus force-pushed the transactions/event-changes branch from b926d0b to 7f48e13 Compare December 19, 2023 10:48

alcaeus requested review from jmikola and malarzm December 19, 2023 10:49

alcaeus force-pushed the feature/transactions branch from 18a61c2 to e728816 Compare December 19, 2023 13:22

alcaeus force-pushed the transactions/event-changes branch from 7f48e13 to a7de8a8 Compare December 19, 2023 13:26

malarzm approved these changes Dec 19, 2023

View reviewed changes

jmikola approved these changes Dec 20, 2023

View reviewed changes

alcaeus force-pushed the feature/transactions branch from e728816 to 601714f Compare January 8, 2024 13:22

alcaeus added 9 commits January 8, 2024 14:23

Pass session and transaction information to event args

14947fb

Only dispatch lifecycle events once per commit operation

cd10c58

Remove isInTransaction property in event args

5b6b38a

Split method signature for readability

1ee31d6

Use property promotion for event args classes

0f0969f

Extract construction of eventArgs

b329df6

Inline spl_object_hash calls

7d5f5d0

Avoid injecting test instance

08ab4b6

Add session to commitOptions in persister

701dde2

alcaeus force-pushed the transactions/event-changes branch 2 times, most recently from f529934 to 082df7d Compare January 8, 2024 14:13

Add session assertions in LifecycleEventManager

84e4eb2

alcaeus force-pushed the transactions/event-changes branch from 082df7d to 84e4eb2 Compare January 8, 2024 14:22

alcaeus merged commit 6126986 into doctrine:feature/transactions Jan 8, 2024
20 checks passed

alcaeus deleted the transactions/event-changes branch January 8, 2024 14:36

alcaeus mentioned this pull request Jan 17, 2024

Transaction support #2586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update events to play nice with transactions #2594

Update events to play nice with transactions #2594

alcaeus commented Nov 29, 2023

jmikola Nov 29, 2023

malarzm Dec 17, 2023 •

edited

Loading

alcaeus Dec 19, 2023

malarzm Dec 17, 2023

alcaeus Dec 19, 2023

jmikola Dec 20, 2023 •

edited

Loading

jmikola left a comment

jmikola Dec 20, 2023

jmikola Dec 20, 2023 •

edited

Loading

jmikola Dec 20, 2023

alcaeus Dec 21, 2023

		public readonly bool $isInTransaction = false,
		public readonly ?Session $session = null,

Update events to play nice with transactions #2594

Update events to play nice with transactions #2594

Conversation

alcaeus commented Nov 29, 2023

Summary

jmikola Nov 29, 2023

Choose a reason for hiding this comment

malarzm Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

alcaeus Dec 19, 2023

Choose a reason for hiding this comment

malarzm Dec 17, 2023

Choose a reason for hiding this comment

alcaeus Dec 19, 2023

Choose a reason for hiding this comment

jmikola Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jmikola Dec 20, 2023

Choose a reason for hiding this comment

jmikola Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

jmikola Dec 20, 2023

Choose a reason for hiding this comment

alcaeus Dec 21, 2023

Choose a reason for hiding this comment

malarzm Dec 17, 2023 •

edited

Loading

jmikola Dec 20, 2023 •

edited

Loading

jmikola Dec 20, 2023 •

edited

Loading