diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 002cf661226d..c6b7a573cf2c 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -337,6 +337,20 @@ abstract class BaseConnection implements ConnectionInterface */ protected $transFailure = false; + /** + * The server has already rolled this transaction back (MySQLi errno 1213). + * mysqli::commit() still returns true, so transCommit() must not report success. + */ + protected bool $transServerRolledBack = false; + + /** + * The next statement must open a server transaction before it runs. + * InnoDB ended the previous one and the connection is back in autocommit. + * Errno 1205 does not set this: innodb_rollback_on_timeout decides whether + * that error rolled the transaction back. + */ + protected bool $transNeedsServerRestart = false; + /** * Whether to throw exceptions during transaction */ @@ -1022,6 +1036,7 @@ public function transBegin(bool $testMode = false): bool if ($this->_transBegin()) { $this->transDepth++; + $this->clearServerRolledBackTransaction(); return true; } @@ -1039,7 +1054,24 @@ public function transCommit(): bool } // When transactions are nested we only begin/commit/rollback the outermost ones - if ($this->transDepth > 1 || $this->_transCommit()) { + if ($this->transDepth > 1) { + $this->transDepth--; + + return true; + } + + // InnoDB already removed the victim transaction. mysqli::commit() + // returns true anyway, which would publish later writes and report success. + // Roll back the restarted server transaction so a false return ends the + // transaction, as transComplete() does. Otherwise a caller that retries + // without transRollback() nests inside it and nothing commits. + if ($this->transServerRolledBack) { + $this->transRollback(); + + return false; + } + + if ($this->_transCommit()) { $this->transDepth--; return true; @@ -1061,6 +1093,10 @@ public function transRollback(): bool if ($this->transDepth > 1 || $this->_transRollback()) { $this->transDepth--; + if ($this->transDepth === 0) { + $this->clearServerRolledBackTransaction(); + } + return true; } @@ -1089,6 +1125,37 @@ public function handleTransStatus(): void } } + /** + * Remember that the server has rolled the open transaction back. + */ + protected function markServerRolledBackTransaction(): void + { + $this->transServerRolledBack = true; + $this->transNeedsServerRestart = true; + } + + /** + * True while the next statement must BEGIN before it runs. + */ + protected function serverTransactionRestartPending(): bool + { + return $this->transNeedsServerRestart && $this->transDepth > 0; + } + + /** + * BEGIN succeeded. Clear the restart only now, so a failed BEGIN keeps it pending. + */ + protected function serverTransactionRestarted(): void + { + $this->transNeedsServerRestart = false; + } + + private function clearServerRolledBackTransaction(): void + { + $this->transServerRolledBack = false; + $this->transNeedsServerRestart = false; + } + /** * Begin Transaction */ diff --git a/system/Database/MySQLi/Connection.php b/system/Database/MySQLi/Connection.php index 10e56e944dd4..c8bb0aa3d941 100644 --- a/system/Database/MySQLi/Connection.php +++ b/system/Database/MySQLi/Connection.php @@ -291,6 +291,16 @@ protected function execute(string $sql) } try { + // Errno 1213 ends the server transaction. Begin again before this + // statement so it does not autocommit. A failed BEGIN goes through + // the normal error path below, and the restart stays pending. + // Errno 1205 is not handled: innodb_rollback_on_timeout decides + // whether InnoDB rolled that transaction back. + if ($this->connID instanceof mysqli && $this->serverTransactionRestartPending()) { + $this->connID->begin_transaction(); + $this->serverTransactionRestarted(); + } + return $this->connID->query($this->prepQuery($sql), $this->resultMode); } catch (mysqli_sql_exception $e) { log_message('error', "{message}\nin {exFile} on line {exLine}.\n{trace}", [ @@ -300,6 +310,10 @@ protected function execute(string $sql) 'trace' => render_backtrace($e->getTrace()), ]); + if ($e->getCode() === 1213 && $this->transDepth > 0) { + $this->markServerRolledBackTransaction(); + } + if ($this->DBDebug) { throw new DatabaseException($e->getMessage(), $e->getCode(), $e); } diff --git a/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php new file mode 100644 index 000000000000..921651a83c2e --- /dev/null +++ b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php @@ -0,0 +1,277 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database\Live\MySQLi; + +use CodeIgniter\Database\BaseConnection; +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; +use mysqli; +use PHPUnit\Framework\Attributes\Group; +use Throwable; + +/** + * InnoDB errno 1213 rolls the victim transaction back. These tests lock two + * rows in opposite orders so the CodeIgniter connection is that victim. + * + * Errno 1205 is a different outcome and is not a passing result here. + * + * @internal + */ +#[Group('DatabaseLive')] +final class DeadlockTransactionTest extends CIUnitTestCase +{ + use DatabaseTestTrait; + + protected $migrate = false; + private ?BaseConnection $waiter = null; + private ?mysqli $held = null; + private ?string $probeTable = null; + + protected function setUp(): void + { + $config = config('Database'); + + if ($config->tests['DBDriver'] !== 'MySQLi') { + $this->markTestSkipped('Only MySQLi can complete this test.'); + } + + $this->db = Database::connect($this->DBGroup, false); + + parent::setUp(); + } + + protected function tearDown(): void + { + $this->releaseDeadlock(); + + parent::tearDown(); + } + + public function testFollowingWriteDoesNotAutocommitAndTransCommitReturnsFalse(): void + { + $this->assertDeadlockVictimDoesNotPublishTheNextWrite(false); + } + + public function testFollowingWriteDoesNotAutocommitAndTransCompleteRollsItBack(): void + { + $this->assertDeadlockVictimDoesNotPublishTheNextWrite(true); + } + + public function testFalseTransCommitEndsTheTransactionSoARetryCommits(): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + + $firstCommit = $this->db->transCommit(); + $depthAfter = $this->db->transDepth; + + // A caller that retries after the false commit must get a real transaction, + // not a nested one inside the restarted server transaction. + $this->db->transBegin(); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (103, 'retry')"); + $retryCommit = $this->db->transCommit(); + + $this->assertSame( + ['first_commit' => false, 'depth_after' => 0, 'retry_commit' => true, 'visible' => [103]], + [ + 'first_commit' => $firstCommit, + 'depth_after' => $depthAfter, + 'retry_commit' => $retryCommit, + 'visible' => $this->committedIds($table), + ], + ); + } + + public function testLostConnectionAfterDeadlockDoesNotEscapeAsDriverException(): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + $this->killConnection($this->db); + + // The restart BEGIN fails on the dead socket. That must surface through the + // normal query error path, not as a raw mysqli_sql_exception. + $insert = $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + $this->db->reconnect(); + + $this->assertSame( + ['insert' => false, 'complete' => false], + ['insert' => $insert, 'complete' => $this->db->transComplete()], + ); + } + + private function killConnection(BaseConnection $db): void + { + $threadId = $db->getConnection()->thread_id; + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + $observer->query('KILL ' . $threadId); + + for ($i = 0; $i < 50; $i++) { + $alive = $observer->query('SELECT COUNT(*) AS n FROM information_schema.processlist WHERE id = ' . $threadId)->getRow()->n; + + if ((int) $alive === 0) { + break; + } + + usleep(20000); + } + + $observer->close(); + } + + private function assertDeadlockVictimDoesNotPublishTheNextWrite(bool $useComplete): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + + $this->assertSame(1213, (int) $this->db->error()['code']); + $this->assertSame(1, $this->db->transDepth); + + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + + $visibleBefore = $this->committedIds($table); + $finished = $useComplete ? $this->db->transComplete() : $this->db->transCommit(); + $visibleAfter = $this->committedIds($table); + + $this->assertSame( + ['visible_before' => [], 'finished' => false, 'visible_after' => []], + [ + 'visible_before' => $visibleBefore, + 'finished' => $finished, + 'visible_after' => $visibleAfter, + ], + ); + } + + /** + * Make $this->db the InnoDB deadlock victim. The victim row is id 101. + */ + private function forceDeadlock(string $table): void + { + for ($attempt = 1; $attempt <= 3; $attempt++) { + $this->releaseDeadlock(); + $this->resetProbe($table); + + $this->waiter = Database::connect($this->DBGroup, false); + $this->waiter->initialize(); + $this->waiter->query('SET SESSION innodb_lock_wait_timeout = 3'); + $this->waiter->transStart(); + + for ($i = 0; $i < 20; $i++) { + $this->waiter->query('UPDATE ' . $table . " SET note = 'w{$i}' WHERE id = 1"); + } + + $held = $this->waiter->getConnection(); + $this->assertInstanceOf(mysqli::class, $held); + $this->held = $held; + + // The victim has to lock row 2 before the waiter asks for it. + // Sending that request first lets the waiter take row 2, and the + // victim then waits out innodb_lock_wait_timeout (errno 1205). + $this->db->query('SET SESSION innodb_lock_wait_timeout = 3'); + $this->db->transException(false); + $this->db->transStart(); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (101, 'victim')"); + $this->db->query('UPDATE ' . $table . " SET note = 'held' WHERE id = 2"); + + $this->held->query('UPDATE ' . $table . " SET note = 'waiting' WHERE id = 2", MYSQLI_ASYNC); + usleep(400000); + + $this->db->query('UPDATE ' . $table . " SET note = 'closing' WHERE id = 1"); + + if ((int) $this->db->error()['code'] === 1213) { + return; + } + } + + $this->fail('Precondition: InnoDB did not report errno 1213 on the CodeIgniter connection.'); + } + + private function resetProbe(string $table): void + { + // releaseDeadlock() clears this marker after it drops the table. + // Record the name again before recreating it, including when setup fails. + $this->probeTable = $table; + + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (1, 'one'), (2, 'two')"); + } + + /** + * @return list + */ + private function committedIds(string $table): array + { + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + $rows = $observer->query('SELECT id FROM ' . $table . ' WHERE id > 100 ORDER BY id')->getResultArray(); + $observer->close(); + + $ids = []; + + foreach ($rows as $row) { + $ids[] = (int) $row['id']; + } + + return $ids; + } + + private function releaseDeadlock(): void + { + if ($this->held instanceof mysqli) { + try { + $this->held->reap_async_query(); + } catch (Throwable) { + // The waiting statement already finished, or the socket is closed. + } + + $this->held = null; + } + + if ($this->waiter instanceof BaseConnection) { + if ($this->waiter->transDepth > 0) { + $this->waiter->transRollback(); + } + + $this->waiter->close(); + $this->waiter = null; + } + + if ($this->db instanceof BaseConnection && $this->db->transDepth > 0) { + $this->db->transRollback(); + } + + if ($this->probeTable !== null && $this->db instanceof BaseConnection) { + $this->db->query('DROP TABLE IF EXISTS ' . $this->probeTable); + $this->probeTable = null; + } + } +} diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..25f64ff88ffc 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -32,6 +32,11 @@ Deprecations Bugs Fixed ********** +- **Database:** Fixed MySQLi errno 1213 (``ER_LOCK_DEADLOCK``) so a following + write through ``query()``, ``simpleQuery()`` or the Query Builder on the + victim connection does not autocommit, and ``transCommit()`` rolls back and + returns false after InnoDB has rolled the victim transaction back. Prepared + queries and errno 1205 are unchanged. See `#10649 `_. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` diff --git a/user_guide_src/source/database/transactions.rst b/user_guide_src/source/database/transactions.rst index 5c48f2806240..984ff27a98be 100644 --- a/user_guide_src/source/database/transactions.rst +++ b/user_guide_src/source/database/transactions.rst @@ -145,6 +145,24 @@ if you would like to run transactions manually you can do so as follows: .. note:: Make sure to use ``$this->db->transBegin()`` when running manual transactions, **NOT** ``$this->db->transStart()``. +MySQL Deadlocks +=============== + +InnoDB error 1213 rolls the victim transaction back. CodeIgniter keeps that +transaction open. The next statement run through ``query()``, ``simpleQuery()`` +or the Query Builder on the same connection starts a new server transaction, +so the write does not autocommit. ``transCommit()`` rolls that server +transaction back and returns false, so the transaction has ended when it +returns. ``transComplete()`` also rolls it back. + +Prepared queries are not covered. A deadlock raised by a prepared query is not +detected, and a prepared query run after a deadlock does not start a new server +transaction. + +Error 1205 is not handled this way. ``innodb_rollback_on_timeout`` decides +whether InnoDB rolled that transaction back, and the message is the same +either way. + Nested Transactions ===================