From 2d2d82b338f23da57d379542ee6056fbf5f69e25 Mon Sep 17 00:00:00 2001 From: Martin Hansen Date: Thu, 23 Nov 2023 16:30:10 +0100 Subject: [PATCH] [10.x] Only stage committed transactions (#49093) * Only stage committed transactions Updates the TransactionManager's stageTransactions method to accept a $level parameter, allowing it to only stage the transactions that will actually be committed. The previous implementation staged all existing transactions, which caused the manager's pendingTransactions and committedTransactions properties to be incorrect, leading to unexpected behaviour after a child transaction was committed but before the parent transaction had been committed. * Fix styleci * push failing test * fix failing test * add another failing test * rename test * fix formatting * add another test * rename test * rename test * refactor to parents * small fix * check connection * key current transactions by connection * another fix * call values * formatting * add another test * comment --------- Co-authored-by: Taylor Otwell --- .../Database/Concerns/ManagesTransactions.php | 4 +- .../Database/DatabaseTransactionRecord.php | 11 +- .../Database/DatabaseTransactionsManager.php | 125 +++++++++++++--- .../DatabaseTransactionsManagerTest.php | 25 +++- tests/Database/DatabaseTransactionsTest.php | 12 +- .../DatabaseTransactionsManagerTest.php | 2 +- .../ShouldDispatchAfterCommitEventTest.php | 133 ++++++++++++++++++ 7 files changed, 277 insertions(+), 35 deletions(-) diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php index 0f6aa0239a71..32bd999b1726 100644 --- a/src/Illuminate/Database/Concerns/ManagesTransactions.php +++ b/src/Illuminate/Database/Concerns/ManagesTransactions.php @@ -47,7 +47,7 @@ public function transaction(Closure $callback, $attempts = 1) $this->getPdo()->commit(); } - $this->transactionsManager?->stageTransactions($this->getName()); + $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions); $this->transactions = max(0, $this->transactions - 1); @@ -196,7 +196,7 @@ public function commit() $this->getPdo()->commit(); } - $this->transactionsManager?->stageTransactions($this->getName()); + $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions); $this->transactions = max(0, $this->transactions - 1); diff --git a/src/Illuminate/Database/DatabaseTransactionRecord.php b/src/Illuminate/Database/DatabaseTransactionRecord.php index 4736ee9224d2..c35acb184aa5 100755 --- a/src/Illuminate/Database/DatabaseTransactionRecord.php +++ b/src/Illuminate/Database/DatabaseTransactionRecord.php @@ -18,6 +18,13 @@ class DatabaseTransactionRecord */ public $level; + /** + * The parent instance of this transaction. + * + * @var \Illuminate\Database\DatabaseTransactionRecord + */ + public $parent; + /** * The callbacks that should be executed after committing. * @@ -30,12 +37,14 @@ class DatabaseTransactionRecord * * @param string $connection * @param int $level + * @param \Illuminate\Database\DatabaseTransactionRecord|null $parent * @return void */ - public function __construct($connection, $level) + public function __construct($connection, $level, ?DatabaseTransactionRecord $parent = null) { $this->connection = $connection; $this->level = $level; + $this->parent = $parent; } /** diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index 2914464858e6..a8e634e42ddf 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -20,6 +20,13 @@ class DatabaseTransactionsManager */ protected $pendingTransactions; + /** + * The current transaction. + * + * @var array + */ + protected $currentTransaction = []; + /** * Create a new database transactions manager instance. * @@ -41,32 +48,57 @@ public function __construct() public function begin($connection, $level) { $this->pendingTransactions->push( - new DatabaseTransactionRecord($connection, $level) + $newTransaction = new DatabaseTransactionRecord( + $connection, + $level, + $this->currentTransaction[$connection] ?? null + ) ); + + $this->currentTransaction[$connection] = $newTransaction; } /** - * Rollback the active database transaction. + * Move relevant pending transactions to a committed state. * * @param string $connection - * @param int $level + * @param int $levelBeingCommitted * @return void */ - public function rollback($connection, $level) + public function stageTransactions($connection, $levelBeingCommitted) { + $this->committedTransactions = $this->committedTransactions->merge( + $this->pendingTransactions->filter( + fn ($transaction) => $transaction->connection === $connection && + $transaction->level >= $levelBeingCommitted + ) + ); + $this->pendingTransactions = $this->pendingTransactions->reject( - fn ($transaction) => $transaction->connection == $connection && $transaction->level > $level - )->values(); + fn ($transaction) => $transaction->connection === $connection && + $transaction->level >= $levelBeingCommitted + ); + + if (isset($this->currentTransaction[$connection])) { + $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; + } } /** - * Commit the active database transaction. + * Commit the root database transaction and execute callbacks. * * @param string $connection * @return void */ public function commit($connection) { + // This method is only called when the root database transaction is committed so there + // shouldn't be any pending transactions, but going to clear them here anyways just + // in case. This method could be refactored to receive a level in the future too. + $this->pendingTransactions = $this->pendingTransactions->reject( + fn ($transaction) => $transaction->connection === $connection + )->values(); + [$forThisConnection, $forOtherConnections] = $this->committedTransactions->partition( fn ($transaction) => $transaction->connection == $connection ); @@ -77,35 +109,88 @@ public function commit($connection) } /** - * Register a transaction callback. + * Rollback the active database transaction. * - * @param callable $callback + * @param string $connection + * @param int $newTransactionLevel * @return void */ - public function addCallback($callback) + public function rollback($connection, $newTransactionLevel) { - if ($current = $this->callbackApplicableTransactions()->last()) { - return $current->addCallback($callback); + if ($newTransactionLevel === 0) { + $this->removeAllTransactionsForConnection($connection); + } else { + $this->pendingTransactions = $this->pendingTransactions->reject( + fn ($transaction) => $transaction->connection == $connection && + $transaction->level > $newTransactionLevel + )->values(); + + if ($this->currentTransaction) { + do { + $this->removeCommittedTransactionsThatAreChildrenOf($this->currentTransaction[$connection]); + + $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; + } while ( + isset($this->currentTransaction[$connection]) && + $this->currentTransaction[$connection]->level > $newTransactionLevel + ); + } } - - $callback(); } /** - * Move all the pending transactions to a committed state. + * Remove all pending, completed, and current transactions for the given connection name. * * @param string $connection * @return void */ - public function stageTransactions($connection) + protected function removeAllTransactionsForConnection($connection) { - $this->committedTransactions = $this->committedTransactions->merge( - $this->pendingTransactions->filter(fn ($transaction) => $transaction->connection === $connection) - ); + $this->currentTransaction[$connection] = null; $this->pendingTransactions = $this->pendingTransactions->reject( - fn ($transaction) => $transaction->connection === $connection + fn ($transaction) => $transaction->connection == $connection + )->values(); + + $this->committedTransactions = $this->committedTransactions->reject( + fn ($transaction) => $transaction->connection == $connection + )->values(); + } + + /** + * Remove all transactions that are children of the given transaction. + * + * @param \Illuminate\Database\DatabaseTransactionRecord $transaction + * @return void + */ + protected function removeCommittedTransactionsThatAreChildrenOf(DatabaseTransactionRecord $transaction) + { + [$removedTransactions, $this->committedTransactions] = $this->committedTransactions->partition( + fn ($committed) => $committed->connection == $transaction->connection && + $committed->parent === $transaction ); + + // There may be multiple deeply nested transactions that have already committed that we + // also need to remove. We will recurse down the children of all removed transaction + // instances until there are no more deeply nested child transactions for removal. + $removedTransactions->each( + fn ($transaction) => $this->removeCommittedTransactionsThatAreChildrenOf($transaction) + ); + } + + /** + * Register a transaction callback. + * + * @param callable $callback + * @return void + */ + public function addCallback($callback) + { + if ($current = $this->callbackApplicableTransactions()->last()) { + return $current->addCallback($callback); + } + + $callback(); } /** diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php index e303b82d89cd..a62c03dfb5ef 100755 --- a/tests/Database/DatabaseTransactionsManagerTest.php +++ b/tests/Database/DatabaseTransactionsManagerTest.php @@ -67,8 +67,8 @@ public function testCommittingTransactions() $manager->begin('default', 2); $manager->begin('admin', 1); - $manager->stageTransactions('default'); - $manager->stageTransactions('admin'); + $manager->stageTransactions('default', 1); + $manager->stageTransactions('admin', 1); $manager->commit('default'); $this->assertCount(0, $manager->getPendingTransactions()); @@ -121,7 +121,7 @@ public function testCommittingTransactionsExecutesCallbacks() $manager->begin('admin', 1); - $manager->stageTransactions('default'); + $manager->stageTransactions('default', 1); $manager->commit('default'); $this->assertCount(2, $callbacks); @@ -148,7 +148,7 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection() $callbacks[] = ['admin', 1]; }); - $manager->stageTransactions('default'); + $manager->stageTransactions('default', 1); $manager->commit('default'); $this->assertCount(1, $callbacks); @@ -185,16 +185,29 @@ public function testStageTransactions() $this->assertEquals(1, $pendingTransactions[1]->level); $this->assertEquals('admin', $pendingTransactions[1]->connection); - $manager->stageTransactions('default'); + $manager->stageTransactions('default', 1); $this->assertCount(1, $manager->getPendingTransactions()); $this->assertCount(1, $manager->getCommittedTransactions()); $this->assertEquals('default', $manager->getCommittedTransactions()[0]->connection); - $manager->stageTransactions('admin'); + $manager->stageTransactions('admin', 1); $this->assertCount(0, $manager->getPendingTransactions()); $this->assertCount(2, $manager->getCommittedTransactions()); $this->assertEquals('admin', $manager->getCommittedTransactions()[1]->connection); } + + public function testStageTransactionsOnlyStagesTheTransactionsAtOrAboveTheGivenLevel() + { + $manager = (new DatabaseTransactionsManager); + + $manager->begin('default', 1); + $manager->begin('default', 2); + $manager->begin('default', 3); + $manager->stageTransactions('default', 2); + + $this->assertCount(1, $manager->getPendingTransactions()); + $this->assertCount(2, $manager->getCommittedTransactions()); + } } diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php index 94998f010c01..384f4223fa81 100644 --- a/tests/Database/DatabaseTransactionsTest.php +++ b/tests/Database/DatabaseTransactionsTest.php @@ -64,7 +64,7 @@ public function testTransactionIsRecordedAndCommitted() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default'); + $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); $transactionManager->shouldReceive('commit')->once()->with('default'); $this->connection()->setTransactionManager($transactionManager); @@ -84,7 +84,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default'); + $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); $transactionManager->shouldReceive('commit')->once()->with('default'); $this->connection()->setTransactionManager($transactionManager); @@ -105,7 +105,8 @@ public function testNestedTransactionIsRecordedAndCommitted() $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); $transactionManager->shouldReceive('begin')->once()->with('default', 2); - $transactionManager->shouldReceive('stageTransactions')->twice()->with('default'); + $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); + $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 2); $transactionManager->shouldReceive('commit')->once()->with('default'); $this->connection()->setTransactionManager($transactionManager); @@ -133,8 +134,9 @@ public function testNestedTransactionIsRecordeForDifferentConnectionsdAndCommitt $transactionManager->shouldReceive('begin')->once()->with('default', 1); $transactionManager->shouldReceive('begin')->once()->with('second_connection', 1); $transactionManager->shouldReceive('begin')->once()->with('second_connection', 2); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default'); - $transactionManager->shouldReceive('stageTransactions')->twice()->with('second_connection'); + $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); + $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 1); + $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 2); $transactionManager->shouldReceive('commit')->once()->with('default'); $transactionManager->shouldReceive('commit')->once()->with('second_connection'); diff --git a/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php b/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php index b0add2e650eb..f5655ebc3862 100644 --- a/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php +++ b/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php @@ -42,7 +42,7 @@ public function testItExecutesCallbacksForTheSecondTransaction() $this->assertFalse($testObject->ran); - $manager->stageTransactions('foo'); + $manager->stageTransactions('foo', 1); $manager->commit('foo'); $this->assertTrue($testObject->ran); $this->assertEquals(1, $testObject->runs); diff --git a/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php b/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php index e20d65891a23..5627aaaeb395 100644 --- a/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php +++ b/tests/Integration/Events/ShouldDispatchAfterCommitEventTest.php @@ -130,6 +130,139 @@ public function testItOnlyDispatchesNestedTransactionsEventsAfterTheRootTransact $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); $this->assertTrue(AnotherShouldDispatchAfterCommitTestEvent::$ran); } + + public function testItDoesNotDispatchAfterCommitEventsImmediatelyIfASiblingTransactionIsCommittedFirst() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + DB::transaction(function () { + }); + + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + }); + + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testChildEventsAreNotDispatchedIfParentTransactionFails() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + try { + DB::transaction(function () { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent); + }); + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + + throw new \Exception; + }); + } catch (\Exception $e) { + // + } + + DB::transaction(fn () => true); + + // Should not have ran because parent transaction failed... + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testItHandlesNestedTransactionsWhereTheSecondOneFails() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + Event::listen(AnotherShouldDispatchAfterCommitTestEvent::class, AnotherShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent()); + }); + + try { + DB::transaction(function () { + // This event should not be dispatched since the transaction is going to fail. + Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); + throw new \Exception; + }); + } catch (\Exception $e) { + } + }); + + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertFalse(AnotherShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testChildCallbacksShouldNotBeDispatchedIfTheirParentFails() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + try { + DB::transaction(function () { + DB::transaction(function () { + Event::dispatch(new ShouldDispatchAfterCommitTestEvent()); + }); + + throw new \Exception; + }); + } catch (\Exception $e) { + // + } + }); + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testItHandlesFailuresWithTransactionsTwoLevelsHigher() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + Event::listen(AnotherShouldDispatchAfterCommitTestEvent::class, AnotherShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { // lv 1 + DB::transaction(function () { // lv 2 + DB::transaction(fn () => Event::dispatch(new ShouldDispatchAfterCommitTestEvent())); + // lv 2 + }); + + try { + DB::transaction(function () { // lv 2 + // This event should not be dispatched since the transaction is going to fail. + Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); + throw new \Exception; + }); + } catch (\Exception $e) { + } + }); + + $this->assertTrue(ShouldDispatchAfterCommitTestEvent::$ran); + $this->assertFalse(AnotherShouldDispatchAfterCommitTestEvent::$ran); + } + + public function testCommittedTransactionThatWasDeeplyNestedIsRemovedIfTopLevelFails() + { + Event::listen(ShouldDispatchAfterCommitTestEvent::class, ShouldDispatchAfterCommitListener::class); + + DB::transaction(function () { + try { + DB::transaction(function () { + DB::transaction(function () { + DB::transaction(function () { + DB::transaction(fn () => Event::dispatch(new ShouldDispatchAfterCommitTestEvent())); + }); + }); + + throw new \Exception; + }); + } catch (\Exception $e) { + + } + }); + + $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran); + } } class TransactionUnawareTestEvent