diff --git a/system/Database/MySQLi/Connection.php b/system/Database/MySQLi/Connection.php index 10e56e944dd4..8d216e6ed142 100644 --- a/system/Database/MySQLi/Connection.php +++ b/system/Database/MySQLi/Connection.php @@ -91,6 +91,16 @@ class Connection extends BaseConnection */ public $foundRows = false; + /** + * InnoDB errno 1213 ended this transaction. + */ + private bool $transServerRolledBack = false; + + /** + * The next statement must BEGIN before it runs. + */ + private bool $transNeedsServerRestart = false; + /** * Connect to the database. * @@ -291,6 +301,18 @@ protected function execute(string $sql) } try { + // 1213 ended the server transaction. BEGIN again so this statement + // does not autocommit. A failed BEGIN stays pending. 1205 is unchanged: + // innodb_rollback_on_timeout decides that case. + if ($this->connID instanceof mysqli && $this->serverTransactionRestartPending()) { + // Exception reporting off: a failed BEGIN returns false. + if (! $this->connID->begin_transaction()) { + return false; + } + + $this->transNeedsServerRestart = false; + } + 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 +322,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); } @@ -574,16 +600,53 @@ public function insertID(): int protected function _transBegin(): bool { - return $this->connID->begin_transaction(); + if (! $this->connID->begin_transaction()) { + return false; + } + + $this->clearDeadlockTransactionState(); + + return true; } protected function _transCommit(): bool { + // mysqli::commit() returns true after 1213. End the transaction + // and report failure instead. + if ($this->transServerRolledBack) { + $this->transRollback(); + + return false; + } + return $this->connID->commit(); } protected function _transRollback(): bool { - return $this->connID->rollback(); + if (! $this->connID->rollback()) { + return false; + } + + $this->clearDeadlockTransactionState(); + + return true; + } + + private function markServerRolledBackTransaction(): void + { + $this->transServerRolledBack = true; + $this->transNeedsServerRestart = true; + } + + private function serverTransactionRestartPending(): bool + { + return $this->transNeedsServerRestart && $this->transDepth > 0; + } + + private function clearDeadlockTransactionState(): void + { + $this->transServerRolledBack = false; + $this->transNeedsServerRestart = false; } } diff --git a/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php new file mode 100644 index 000000000000..433c982a6403 --- /dev/null +++ b/tests/system/Database/Live/MySQLi/DeadlockTransactionTest.php @@ -0,0 +1,304 @@ + + * + * 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 mysqli_driver; +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()], + ); + } + + public function testRestartThatCannotBeginDoesNotRunTheStatement(): void + { + $table = $this->db->escapeIdentifier($this->db->prefixTable('deadlock_probe')); + $this->probeTable = $table; + $this->forceDeadlock($table); + + // An active XA transaction makes MySQL refuse START TRANSACTION on a live + // connection. With mysqli exception reporting off, begin_transaction() + // then returns false instead of throwing. + $mysqli = $this->db->getConnection(); + $previous = (new mysqli_driver())->report_mode; + $mysqli->query("XA START 'deadlock_probe'"); + mysqli_report(MYSQLI_REPORT_OFF); + + try { + $insert = $this->db->query('INSERT INTO ' . $table . " (id, note) VALUES (102, 'after')"); + $inserted = (int) $mysqli->query('SELECT COUNT(*) FROM ' . $table . ' WHERE id = 102')->fetch_row()[0]; + } finally { + $mysqli->query("XA END 'deadlock_probe'"); + $mysqli->query("XA ROLLBACK 'deadlock_probe'"); + mysqli_report($previous); + } + + $this->assertSame(['insert' => false, 'inserted' => 0], ['insert' => $insert, 'inserted' => $inserted]); + } + + 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 bf2a9bc92987..30b02dfa9ed4 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -39,6 +39,7 @@ Bugs Fixed - **CLI:** Fixed a bug where a prompt asked again forever when its input was piped or redirected and ran out before a valid answer: ``CLI::prompt()`` and ``CLI::promptByKey()`` whose default fails the validation rules, and ``CLI::promptByMultipleKeys()`` whose options have string keys. They now throw a ``CLIException``. The new ``InputOutput::isAtEnd()`` tells that end of the input from an empty line. Pressing Ctrl+D at a terminal still asks again. - **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings. - **Database:** Fixed ``reconnect()`` so a replaced socket does not keep the transaction that died with the old connection. ``transDepth`` is cleared and the transaction status is failed before the new connection is opened, so a failed reconnect cannot still report success. OCI8 commit mode returns to autocommit. ``transComplete()`` does not report success. This applies when ``ping()`` reports the connection lost; PostgreSQL's ``pg_ping()`` reconnects a broken connection itself, so it is not covered. +- **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. - **Format:** Fixed ``XMLFormatter`` so that an array key that becomes empty after sanitization no longer triggers a ``ValueError``; the fallback tag name ``item`` is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. - **Helpers:** Fixed a regression in v4.7.5 where ``set_value()`` threw a ``TypeError`` when the default value was not a string or an array, such as an integer, a float, ``null``, or a ``Stringable`` object. Such values are now converted to a string. - **HTTP:** Fixed a bug in ``ContentSecurityPolicy`` where a numeric source such as ``'0'`` threw a ``TypeError``, and a space-separated directive string in ``Config\\ContentSecurityPolicy`` was emitted as one unquoted source. diff --git a/user_guide_src/source/database/transactions.rst b/user_guide_src/source/database/transactions.rst index 5c48f2806240..96cd6d15d130 100644 --- a/user_guide_src/source/database/transactions.rst +++ b/user_guide_src/source/database/transactions.rst @@ -145,6 +145,26 @@ 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. The rollback happens when the outermost +transaction finishes: ``transCommit()`` rolls that server transaction back and +returns false, so the transaction has ended when it returns, and +``transComplete()`` also rolls it back. At a nested depth, ``transCommit()`` +still returns true and only decrements the depth. + +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 ===================