[10.x] Fix "after commit" callbacks not running on nested transactions #48466

tonysm · 2023-09-20T04:53:01Z

Changed

Deprecates the Illuminate\Database\DatabaseTransactionsManager::callbackApplicableTransactions method (no longer used in the framework)
Deprecates the Illuminate\Database\DatabaseTransactionsManager::callbacksShouldIgnore method (no longer used in the framework. We're now using the new withAfterCommitCallbacksInTestTransactionAwareMode method, which this one is forwarding to
Adds a new Illuminate\Database\DatabaseTransactionsManager::withAfterCommitCallbacksInTestTransactionAwareMode method meant to be used by the test database traits when they start a transaction. This should make sure that we execute the callbacks properly at the right level of the transaction (since the tests using the database traits start a wrapping transaction and never actually commit)
Adds a new param to the Illuminate\Database\DatabaseTransactionsManager::commit($connection, $level = 1) method. Now, instead of only having to pass the connection name, callers should also pass the transaction level. It's based on that level (and the fact that we're in test mode or not) that we're going to decide if the callbacks should run or not). All calls within the framework were updated.
Also, the Illuminate\Database\DatabaseTransactionsManager::commit($connection, $level = 1) is now called everytime there's a commit. Before it was only being called on the "last one", but now it receives the transaction level and it should decide if the callbacks should be triggered or not.

A More In-Depth Explanation

Changes the way "after commit" callbacks are executed to avoid
remembering which transactions to ignore.

Before, we remembered the test transaction, so we
could ignore it when deciding to run the "after commit"
callbacks or not.

We're still handling the "after commit" callbacks like that,
but now, instead of remembering which transactions to ignore,
we're always calling the DatabaseTransactionManager::commit
method. The difference is that now we're passing the current
transaction level to it. The method will decide whether to run the
callbacks based on that level and whether or not this
is in test mode.

When in tests, instead of setting the current transaction to be
remembered so it could be ignored, we're now only setting the
DatabaseTransactionManager to test mode. When in test mode, it
will execute the callbacks when the transaction count reaches 1
(remember that the test runs in a transaction, so that's the
"root" level). Otherwise, it runs the callbacks when the transaction
level is on level 0 (like in production).

There's also a change in the DatabaseTransactionManager::addCallback
method. It now also checks if it's in test mode. When not in test mode,
it only adds the callback to the execution queue if there's an open
transaction. Otherwise, the callback is executed right away. When in
test mode, the number of transactions has to be greater than one for
it to be added to the callbacks queue.

Context

With the addition of the withSavepointIfNeeded in #48144, we added a new transition level that affects tests when you're using "after commit" callbacks and nesting transactions. This wasn't an issue before because we generally have one transaction wrapping. When the createOrFirst was added to other *OrCreate methods (firstOrCreate and updateOrCreate), this issue became more apparent.

fixes #48451

src/Illuminate/Database/Connection.php

tonysm · 2023-09-20T05:20:16Z

I've removed the test I added here because the build was failing (even though the test passes locally in isolation - the suite also fails locally when running it entirely). I added that test because I wanted a way to test the framework as an application test was using it. Since this issue only appears in tests, I'm unsure if this is the right way. Ignoring one transaction level was a good solution because only the test code was added to the transition to the ignore list. And now we're calling that inside the framework itself... 🤔

tonysm · 2023-09-20T05:27:59Z

I marked it as ready for review so I could get some feedback:

Is it okay that the withSavepointIfNeeded now adds the savepoint transition to the list of transactions that should be ignored when deciding whether to run the callbacks (using $afterCommit on Observers, for instance)
Does anyone have some ideas on how to test this? I added a test here, but that broke the build. I see some tests setting the db.transactions manually to a mocked like the EventListenerTest, which might be the way to go. I ran out of time tonight.

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

crynobone · 2023-09-20T06:24:36Z

@tonysm added tests based on your example as Integration tests

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

src/Illuminate/Database/DatabaseTransactionsManager.php

mpyw · 2023-09-20T08:56:06Z

After careful reconsideration, I believe this corrective approach is incorrect. I consider the following logic to be highly suspect:

if ($this->transactions == 1) {
    $this->fireConnectionEvent('committing');
    $this->getPdo()->commit();
}

$this->transactions = max(0, $this->transactions - 1);

if ($this->afterCommitCallbacksShouldBeExecuted()) {
    $this->transactionsManager?->commit($this->getName());
}

The root cause of the inconsistency is that we are only decrementing $this->transactions and not releasing the transaction object from the TransactionManager. We need to merge the registered callbacks into the callback list of the higher-level transaction and release the transaction object at that time. Otherwise, the counts will not match.

I'm now preparing a new PR, testing to see if this conjecture is correct.

tonysm · 2023-09-20T12:43:36Z

@crynobone thank you!

Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>

taylorotwell · 2023-09-20T15:02:17Z

I'm starting to worry we are digging a deeper and deeper complicated hole for ourselves after the first createOrFirst PR. It seems we keep needing to come back and add additional complicated PRs to fix new bugs we are creating each time, and I don't know how to proceed with confidence on any additional PRs.

@tonysm and comments on @mpyw's note?

mpyw · 2023-09-20T15:31:21Z

Hi @taylorotwell,

Indeed, createOrFirst seems to have become a trigger for these issues. However, based on my analysis, I believe the root cause of the current problem is not createOrFirst itself, but the interoperability issues between afterCommit (Added by @themsaid) and RefreshDatabase.

There are indeed because of RefreshDatabase + double calls of DB::transaction(). This happens regardless of the createOrFirst() implementation.
@tonysm has proposed a temporary fix with this PR, but I'm deeply concerned that adopting this change will unnecessarily complicate createOrFirst even further. It's undoubtedly insufficient as a permanent solution.
While you, @taylorotwell, have already addressed the issue in [9.x] Fix afterCommit and RefreshDatabase #41782, there are still problems:
- Nested transactions were overlooked, leading to the issue in RefreshDatabase + double wrapping in DB::transaction() breaks $afterCommit functionality #48451.
- Multiple connections were not considered, causing the problem in afterCommit() callbacks do not fire when RefreshDatabase handles multiple $connectionsToTransact #48472.

As a long-term solution, I started trying to fix it with the approach in #48471. However, the changes seem to be getting quite complex, and I'm at my wit's end. The problem is becoming too challenging for me to handle. I'd like to request someone else to create a new PR and continue the work 😢

tonysm · 2023-09-20T21:49:06Z

@taylorotwell I feel you. I agree with @mpyw. I think createOrFirst on its own was okay. Adding it to firstOrCreate made sense to me, but with the complications of the savepoints, I started to think it was perhaps a deeper undertaking than I anticipated. Then, it was added to the updateOrCreate method, which seems to bring another set of issues.

But in this case, the issue seems to be with test transactions and the afterCommit feature. The savepoints added for createOrFirst only surfaced the issue.

This is not an issue in production apps. It only affects testing things that use afterCommit callbacks and when using transactions (which is why that feature exists). When createOrFirst was added to other methods, it revealed the issue.

I've turned the PR back to draft. I have an idea of how this can be fixed: we could have a test-specific transaction manager that would not count the test wrapping transaction when deciding if the callbacks should run.

to avoid remembering which transactions to ignore Before, we were remembering the test transaction so we could ignore it when deciding to run the after commit callbacks or not. We're still handling the after commit callbacks like that but now instead of remembering which transactions to ignore, we're always calling the DatabaseTransactionManager::commit method. The difference is that now we're passing the current transaction level to it. The method will decide to call the callbacks or not based on that level and whether or not this is on in test mode. When in tests, instead of setting the current transaction to be remembered so it could be ignored, we're now only setting the DatabaseTransactionManager to test mode. When in test mode, it will execute the callbacks when the transactions count reaches 1 (remember that the test runs in a transaction, so that's the "root" level). Otherwise, it runs the callbacks when the transactions level is on level 0 (like in production). There's also a change in the DatabaseTransactionManager::addCallback method. It now also checks if it's in test mode. When not in test mode, it only adds the callback to the execution queue if there's an open transaction. Otherwise, the callback is executed right away. When in test mode, the number of transactions has to be greater than one for it to be added to the callbacks queue.

mpyw · 2023-09-21T00:50:55Z

@tonysm

we could have a test-specific transaction manager

I agree with that!

src/Illuminate/Database/DatabaseTransactionsManager.php

tonysm · 2023-09-21T02:12:14Z

Ready for review. \cc @taylorotwell @crynobone @mpyw

src/Illuminate/Database/DatabaseTransactionsManager.php

taylorotwell · 2023-09-21T21:01:21Z

@mpyw what are your thoughts on the overall state of this PR now?

mpyw · 2023-09-22T03:10:45Z

@taylorotwell

While I believe it's okay to merge in terms of functionality, there might be room for renaming and structural refactoring. I'd like to leave the final decision up to you.

My thoughts:

Rename `withAfterCommitCallbacksInTestMode` to `withAfterCommitCallbacksInDatabaseTransactionsTestMode` `withAfterCommitCallbacksInTestTransactionAwareMode`

Just calling it "test mode" gives the impression that it applies to all tests. However, it's more accurate to specify that it's for the "test mode using the DatabaseTransactions trait." Even if it's a bit verbose, I thought a more precise naming would be better.

withAfterCommitCallbacksInTestTransactionAwareMode would be better

Separate the test implementation into a different class

Ideally, the implementation used in production and the one for testing should be distinct. Instead of introducing methods like withAfterCommitCallbacksInDatabaseTransactionsTestMode to the Connection class, I thought it might be better to provide a TestDatabaseTransactionsManager (or TestTransactionAwareDatabaseTransactionsManager) for testing purposes and switch the implementation using setter injection. Given that we have a setter injection mechanism in place, I believe it would be more rational to utilize it. The implementation for testing can be easily created by inheriting the production implementation and overriding some of the methods.

mpyw · 2023-09-22T03:22:07Z

@taylorotwell

Separate the test implementation into a different class

Like this:

`$level == 1 || ($this->afterCommitCallbacksInTestMode && $level == 2))`

`DatabaseTransactionsManager`

protected function isLowestTransactionLevel(int $level): bool
{
    return $level <= 1; // or $level == 1
}

`TestTransactionAwareDatabaseTransactionsManager`

protected function isLowestTransactionLevel(int $level): bool
{
    return $level <= 2; // or $level == 2
}

`$this->transactions->isEmpty() || ($this->afterCommitCallbacksInTestMode && $this->transactions->count() == 1)`

`DatabaseTransactionsManager`

protected function transactionsAreEmpty(): bool
{
    return $this->transactions->isEmpty();
}

`TestTransactionAwareDatabaseTransactionsManager`

protected function transactionsAreEmpty(): bool
{
    return $this->transactions->count() <= 1; // or $this->transactions->count() == 1
}

Example Diff

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
index fd949cf055..fa2e5517ba 100755
--- a/src/Illuminate/Database/DatabaseTransactionsManager.php
+++ b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -11,13 +11,6 @@ class DatabaseTransactionsManager
      */
     protected $transactions;
 
-    /**
-     * When in test mode, we'll run the after commit callbacks on the top-level transaction.
-     *
-     * @var bool
-     */
-    protected $afterCommitCallbacksInTestMode = false;
-
     /**
      * Create a new database transactions manager instance.
      *
@@ -26,19 +19,6 @@ class DatabaseTransactionsManager
     public function __construct()
     {
         $this->transactions = collect();
-        $this->afterCommitCallbacksInTestMode = false;
-    }
-
-    /**
-     * Sets the transaction manager to test mode.
-     *
-     * @return self
-     */
-    public function withAfterCommitCallbacksInTestMode()
-    {
-        $this->afterCommitCallbacksInTestMode = true;
-
-        return $this;
     }
 
     /**
@@ -78,10 +58,7 @@ public function rollback($connection, $level)
      */
     public function commit($connection, $level = 1)
     {
-        // If the transaction level being commited reaches 1 (meaning it was the root
-        // transaction), we'll run the callbacks. In test mode, since we wrap each
-        // test in a transaction, we'll run the callbacks when reaching level 2.
-        if ($level == 1 || ($this->afterCommitCallbacksInTestMode && $level == 2)) {
+        if ($this->isLowestTransactionLevel($level)) {
             [$forThisConnection, $forOtherConnections] = $this->transactions->partition(
                 fn ($transaction) => $transaction->connection == $connection
             );
@@ -100,10 +77,7 @@ public function commit($connection, $level = 1)
      */
     public function addCallback($callback)
     {
-        // If there are no transactions, we'll run the callbacks right away. Also, we'll run it
-        // right away when we're in test mode and we only have the wrapping transaction. For
-        // every other case, we'll queue up the callback to run after the commit happens.
-        if ($this->transactions->isEmpty() || ($this->afterCommitCallbacksInTestMode && $this->transactions->count() == 1)) {
+        if ($this->transactionsAreEmpty()) {
             return $callback();
         }
 
@@ -119,4 +93,21 @@ public function getTransactions()
     {
         return $this->transactions;
     }
+
+    /**
+     * @param  int  $level
+     * @return bool
+     */
+    protected function isLowestTransactionLevel($level)
+    {
+        return $level <= 1;
+    }
+
+    /**
+     * @return bool
+     */
+    protected function transactionsAreEmpty()
+    {
+        return $this->transactions->isEmpty();
+    }
 }
diff --git a/src/Illuminate/Foundation/Testing/TestTransactionAwareDatabaseTransactionsManager.php b/src/Illuminate/Foundation/Testing/TestTransactionAwareDatabaseTransactionsManager.php
new file mode 100644
index 0000000000..e0ba944736
--- /dev/null
+++ b/src/Illuminate/Foundation/Testing/TestTransactionAwareDatabaseTransactionsManager.php
@@ -0,0 +1,25 @@
+<?php
+
+namespace Illuminate\Foundation\Testing;
+
+use Illuminate\Database\DatabaseTransactionsManager;
+
+class TestTransactionAwareDatabaseTransactionsManager extends DatabaseTransactionsManager
+{
+    /**
+     * @param  int  $level
+     * @return bool
+     */
+    protected function isLowestTransactionLevel($level)
+    {
+        return $level <= 2;
+    }
+
+    /**
+     * @return bool
+     */
+    protected function transactionsAreEmpty()
+    {
+        return $this->transactions->count() <= 1;
+    }
+}

However, since 'db.transactions' is instantiated from various sources, it may be difficult to override it in the container. We will need to replace it at a very early stage of each test if RefreshDatabase DatabaseTransactions traits are used.

mpyw · 2023-09-22T09:59:29Z

After giving it some thought, I've come to believe that just renaming the method might be sufficient, considering the feasibility. I think the method name withAfterCommitCallbacksInTestTransactionAwareMode (and similarly named properties) would be good! Separating the test implementation into a different class doesn't seem realistic, so this should suffice.

tonysm · 2023-09-22T12:48:31Z

I'll rename the method in a bit.

src/Illuminate/Database/DatabaseTransactionsManager.php

tonysm · 2023-09-22T13:18:56Z

src/Illuminate/Database/DatabaseTransactionsManager.php

-     *
-     * @return \Illuminate\Support\Collection
-     */
-    public function callbackApplicableTransactions()


This one too, should I add back the signature? We could probably implement it to return all transactions when not in test mode and, if in test mode, skip the first one.

I've added this method back with an implementation I believe should work the same as the previous one. We're also marking the method as deprecated.

…as deprecated

mpyw

LGTM

tonysm · 2023-09-23T17:48:50Z

By the way, if we get one more issue related to createOrFirst, I think we should revert from using it in the other *OrCreate methods. I can send a PR for that if that happens.

mpyw · 2023-09-24T06:48:42Z

I understand your concern, but I really hope that there won't be any more serious issues. The bug this time was due to a lack of consideration on the afterCommit side, so I don't think it's fair to attribute it to the modifications in the *OrCreate methods.

I feel this change has benefited many applications. It even obviated the need for a third-party library I used to operate with, so I hope the results of this modification will persist.

tonysm · 2023-09-25T01:41:42Z

I also see the benefits of using it in the other methods. It's just that it had some unexpected side effects, unfortunately. No blaming. Just wanted to say that we have that as an option if we have to.

tonysm · 2023-09-25T15:22:17Z

Closing it as @crynobone has an improved version with distinct DatabaseTransactionsManager classes between test and production at #48523

tonysm added 4 commits September 20, 2023 00:56

Tests observer using afterCommit

1f10b5e

Adds failing test for savepoint and observers using afterCommit

d9538d6

Ignore the createOrFirst savepoint to run callbacks

e6de881

Fix typo

06adc98

tonysm mentioned this pull request Sep 20, 2023

RefreshDatabase + double wrapping in DB::transaction() breaks $afterCommit functionality #48451

Closed

tonysm commented Sep 20, 2023

View reviewed changes

src/Illuminate/Database/Connection.php Outdated Show resolved Hide resolved

Remove DatabaseEloquentAppTest to see if CI passes

bf2764b

tonysm force-pushed the fix-create-or-first-transaction-callbacks branch from 8f0232a to bf2764b Compare September 20, 2023 05:09

tonysm marked this pull request as ready for review September 20, 2023 05:28

crynobone added 2 commits September 20, 2023 14:21

wip

451fd8b

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

wip

f49260e

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

wip

a7a2fa2

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>

crynobone reviewed Sep 20, 2023

View reviewed changes

src/Illuminate/Database/DatabaseTransactionsManager.php Outdated Show resolved Hide resolved

mpyw mentioned this pull request Sep 20, 2023

[PoC] [10.x] Fix nested transaction callbacks handling #48471

Closed

Update src/Illuminate/Database/DatabaseTransactionsManager.php

e3df8b1

Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>

tonysm marked this pull request as draft September 20, 2023 21:14

tonysm added 3 commits September 20, 2023 22:00

Fix DatabaseTransactionsTest

1d7b8ee

Fix DatabaseTransactionsManagerTest

65c75ae

CSFixer

2037453

tonysm changed the title ~~[10.x] Fix createOrFirst not running callbacks on tests~~ [10.x] Fix "after commit" callbacks not running on nested transactions Sep 21, 2023

tonysm added 3 commits September 20, 2023 22:32

wip

c7c7f3a

Rename method and property and inline usage

0d8f1fa

Adds a depply nested transaction test

393a69c

crynobone reviewed Sep 21, 2023

View reviewed changes

src/Illuminate/Database/DatabaseTransactionsManager.php Outdated Show resolved Hide resolved

Simplify deeply nesting test

d3dcb03

tonysm marked this pull request as ready for review September 21, 2023 02:11

Sets default level value to one (since it's a new parameter)

68ed29a

mpyw reviewed Sep 21, 2023

View reviewed changes

src/Illuminate/Database/DatabaseTransactionsManager.php Outdated Show resolved Hide resolved

mpyw mentioned this pull request Sep 21, 2023

afterCommit() callbacks do not fire when RefreshDatabase handles multiple $connectionsToTransact #48472

Closed

Rename method

5825929

tonysm commented Sep 22, 2023

View reviewed changes

src/Illuminate/Database/DatabaseTransactionsManager.php Show resolved Hide resolved

tonysm commented Sep 22, 2023

View reviewed changes

tonysm added 3 commits September 22, 2023 10:36

Adds back the removed methods from the db.transactions and mark them …

c660879

…as deprecated

StyleCI

e111e70

Inline the if statement using then collection when() method

239e6ec

mpyw reviewed Sep 23, 2023

View reviewed changes

wip

116f9de

crynobone mentioned this pull request Sep 25, 2023

[10.x] Fix "after commit" callbacks not running on nested transactions using RefreshDatabase or DatabaseMigrations #48523

Merged

tonysm closed this Sep 25, 2023

tonysm deleted the fix-create-or-first-transaction-callbacks branch September 25, 2023 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[10.x] Fix "after commit" callbacks not running on nested transactions #48466

[10.x] Fix "after commit" callbacks not running on nested transactions #48466

tonysm commented Sep 20, 2023 •

edited

tonysm commented Sep 20, 2023

tonysm commented Sep 20, 2023

crynobone commented Sep 20, 2023

mpyw commented Sep 20, 2023 •

edited

tonysm commented Sep 20, 2023

taylorotwell commented Sep 20, 2023 •

edited

mpyw commented Sep 20, 2023 •

edited

tonysm commented Sep 20, 2023

mpyw commented Sep 21, 2023

tonysm commented Sep 21, 2023

taylorotwell commented Sep 21, 2023

mpyw commented Sep 22, 2023 •

edited

mpyw commented Sep 22, 2023 •

edited

Separate the test implementation into a different class

mpyw commented Sep 22, 2023

tonysm commented Sep 22, 2023

tonysm Sep 22, 2023

tonysm Sep 22, 2023 •

edited

mpyw left a comment

tonysm commented Sep 23, 2023

mpyw commented Sep 24, 2023 •

edited

tonysm commented Sep 25, 2023

tonysm commented Sep 25, 2023 •

edited

[10.x] Fix "after commit" callbacks not running on nested transactions #48466

[10.x] Fix "after commit" callbacks not running on nested transactions #48466

Conversation

tonysm commented Sep 20, 2023 • edited

Changed

A More In-Depth Explanation

Context

tonysm commented Sep 20, 2023

tonysm commented Sep 20, 2023

crynobone commented Sep 20, 2023

mpyw commented Sep 20, 2023 • edited

tonysm commented Sep 20, 2023

taylorotwell commented Sep 20, 2023 • edited

mpyw commented Sep 20, 2023 • edited

tonysm commented Sep 20, 2023

mpyw commented Sep 21, 2023

tonysm commented Sep 21, 2023

taylorotwell commented Sep 21, 2023

mpyw commented Sep 22, 2023 • edited

Rename withAfterCommitCallbacksInTestMode to withAfterCommitCallbacksInDatabaseTransactionsTestMode withAfterCommitCallbacksInTestTransactionAwareMode

Separate the test implementation into a different class

mpyw commented Sep 22, 2023 • edited

Separate the test implementation into a different class

$level == 1 || ($this->afterCommitCallbacksInTestMode && $level == 2))

DatabaseTransactionsManager

TestTransactionAwareDatabaseTransactionsManager

$this->transactions->isEmpty() || ($this->afterCommitCallbacksInTestMode && $this->transactions->count() == 1)

DatabaseTransactionsManager

TestTransactionAwareDatabaseTransactionsManager

mpyw commented Sep 22, 2023

tonysm commented Sep 22, 2023

tonysm Sep 22, 2023

Choose a reason for hiding this comment

tonysm Sep 22, 2023 • edited

Choose a reason for hiding this comment

mpyw left a comment

Choose a reason for hiding this comment

tonysm commented Sep 23, 2023

mpyw commented Sep 24, 2023 • edited

tonysm commented Sep 25, 2023

tonysm commented Sep 25, 2023 • edited

tonysm commented Sep 20, 2023 •

edited

mpyw commented Sep 20, 2023 •

edited

taylorotwell commented Sep 20, 2023 •

edited

mpyw commented Sep 20, 2023 •

edited

mpyw commented Sep 22, 2023 •

edited

Rename `withAfterCommitCallbacksInTestMode` to `withAfterCommitCallbacksInDatabaseTransactionsTestMode` `withAfterCommitCallbacksInTestTransactionAwareMode`

mpyw commented Sep 22, 2023 •

edited

`$level == 1 || ($this->afterCommitCallbacksInTestMode && $level == 2))`

`DatabaseTransactionsManager`

`TestTransactionAwareDatabaseTransactionsManager`

`$this->transactions->isEmpty() || ($this->afterCommitCallbacksInTestMode && $this->transactions->count() == 1)`

`DatabaseTransactionsManager`

`TestTransactionAwareDatabaseTransactionsManager`

tonysm Sep 22, 2023 •

edited

mpyw commented Sep 24, 2023 •

edited

tonysm commented Sep 25, 2023 •

edited