diff --git a/system/Database/BaseConnection.php b/system/Database/BaseConnection.php index 002cf661226d..e71645e0f9c7 100644 --- a/system/Database/BaseConnection.php +++ b/system/Database/BaseConnection.php @@ -625,16 +625,44 @@ public function close() * Keep or establish the connection if no queries have been sent for * a length of time exceeding the server's idle timeout. * + * A replaced socket is not in the transaction that was open on the old + * one. The server has already rolled that work back. Clear the depth and + * mark the transaction failed before opening the new connection, so a + * failed reconnect cannot still report success. OCI8 also returns its + * commit mode to autocommit. + * * @return void */ public function reconnect() { if ($this->ping() === false) { + $lostTransaction = $this->transDepth > 0; + $this->close(); + + // initialize() can throw. Clear the lost transaction first so a + // failed reconnect cannot still report the old transaction. + if ($lostTransaction) { + $this->abandonLostTransaction(); + } + $this->initialize(); } } + /** + * The socket that owned the transaction is already closed. + * + * Drivers that keep transaction state in PHP, rather than only on the + * server, must override this. OCI8 commit mode is one case: leaving it + * at OCI_NO_AUTO_COMMIT makes later writes uncommitted. + */ + protected function abandonLostTransaction(): void + { + $this->transDepth = 0; + $this->transStatus = false; + } + /** * Platform dependent way method for closing the connection. * diff --git a/system/Database/OCI8/Connection.php b/system/Database/OCI8/Connection.php index 0d700ef095cf..69f5d98e6416 100644 --- a/system/Database/OCI8/Connection.php +++ b/system/Database/OCI8/Connection.php @@ -722,6 +722,13 @@ protected function _transRollback(): bool return oci_rollback($this->connID); } + protected function abandonLostTransaction(): void + { + $this->commitMode = OCI_COMMIT_ON_SUCCESS; + + parent::abandonLostTransaction(); + } + /** * Returns the name of the current database being used. */ diff --git a/tests/system/Database/Live/ReconnectDuringTransactionTest.php b/tests/system/Database/Live/ReconnectDuringTransactionTest.php new file mode 100644 index 000000000000..280d68d6f034 --- /dev/null +++ b/tests/system/Database/Live/ReconnectDuringTransactionTest.php @@ -0,0 +1,132 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database\Live; + +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\DatabaseTestTrait; +use Config\Database; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('DatabaseLive')] +final class ReconnectDuringTransactionTest extends CIUnitTestCase +{ + use DatabaseTestTrait; + + protected $migrate = false; + + protected function setUp(): void + { + // Dedicated connection so a forced wait_timeout cannot drop the shared one. + $this->db = Database::connect($this->DBGroup, false); + + parent::setUp(); + } + + public function testReconnectInsideTransactionDoesNotReportSuccess(): void + { + if ($this->db->DBDriver !== 'MySQLi') { + $this->markTestSkipped('Requires the MySQLi driver.'); + } + + $table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe')); + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL AUTO_INCREMENT, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + + $observer = Database::connect($this->DBGroup, false); + $observer->initialize(); + + try { + $this->db->transStart(); + $this->db->table('reconnect_probe')->insert(['note' => 'before-timeout']); + $this->assertSame(1, $this->db->transDepth); + $this->assertTrue($this->db->transStatus()); + + $this->db->query('SET SESSION wait_timeout = 1'); + sleep(3); + + $this->assertFalse($this->db->ping(), 'Precondition: the server should have closed the idle socket.'); + + $this->db->reconnect(); + + $depthAfterReconnect = $this->db->transDepth; + $statusAfterReconnect = $this->db->transStatus(); + $completed = $this->db->transComplete(); + $rows = (int) $observer->table('reconnect_probe')->countAllResults(); + + $this->assertSame( + ['depth' => 0, 'status' => false, 'completed' => false, 'rows' => 0], + [ + 'depth' => $depthAfterReconnect, + 'status' => $statusAfterReconnect, + 'completed' => $completed, + 'rows' => $rows, + ], + ); + } finally { + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $observer->close(); + $this->db->close(); + } + } + + public function testReconnectKeepsAnOpenTransactionWhenTheSocketIsAlive(): void + { + if ($this->db->DBDriver !== 'MySQLi') { + $this->markTestSkipped('Requires the MySQLi driver.'); + } + + $table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe')); + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->query( + 'CREATE TABLE ' . $table . ' (' + . 'id INT NOT NULL AUTO_INCREMENT, ' + . 'note VARCHAR(32) NOT NULL, ' + . 'PRIMARY KEY (id)' + . ') ENGINE=InnoDB', + ); + + try { + $this->db->transStart(); + $this->db->table('reconnect_probe')->insert(['note' => 'still-open']); + $this->assertTrue($this->db->ping()); + + $this->db->reconnect(); + + $this->assertSame(1, $this->db->transDepth); + $this->assertTrue($this->db->transStatus()); + $this->assertTrue($this->db->transComplete()); + $this->assertSame(1, $this->db->table('reconnect_probe')->countAllResults()); + } finally { + if ($this->db->transDepth > 0) { + $this->db->transRollback(); + } + + $this->db->query('DROP TABLE IF EXISTS ' . $table); + $this->db->close(); + } + } +} diff --git a/tests/system/Database/ReconnectLostTransactionTest.php b/tests/system/Database/ReconnectLostTransactionTest.php new file mode 100644 index 000000000000..eca053ef5b41 --- /dev/null +++ b/tests/system/Database/ReconnectLostTransactionTest.php @@ -0,0 +1,75 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Database; + +use CodeIgniter\Database\Exceptions\DatabaseException; +use CodeIgniter\Database\MySQLi\Connection as MySQLiConnection; +use CodeIgniter\Database\OCI8\Connection as OCI8Connection; +use CodeIgniter\Test\CIUnitTestCase; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('Others')] +final class ReconnectLostTransactionTest extends CIUnitTestCase +{ + public function testReconnectClearsTransactionStateWhenInitializeFails(): void + { + $db = new class (['DBDriver' => 'MySQLi']) extends MySQLiConnection { + public function initialize(): never + { + throw new DatabaseException('database unavailable'); + } + }; + + $this->setPrivateProperty($db, 'transDepth', 1); + + try { + $db->reconnect(); + $this->fail('initialize() should have thrown.'); + } catch (DatabaseException $exception) { + $this->assertSame('database unavailable', $exception->getMessage()); + } + + $this->assertSame(0, $this->getPrivateProperty($db, 'transDepth')); + $this->assertFalse($db->transStatus()); + } + + public function testReconnectRestoresOci8CommitModeWhenTransactionIsLost(): void + { + if (! defined('OCI_COMMIT_ON_SUCCESS')) { + define('OCI_COMMIT_ON_SUCCESS', 32); + } + + if (! defined('OCI_NO_AUTO_COMMIT')) { + define('OCI_NO_AUTO_COMMIT', 0); + } + + $db = new class (['DBDriver' => 'OCI8']) extends OCI8Connection { + public function initialize(): void + { + } + }; + + $db->commitMode = OCI_NO_AUTO_COMMIT; + $this->setPrivateProperty($db, 'transDepth', 1); + + $db->reconnect(); + + $this->assertSame(OCI_COMMIT_ON_SUCCESS, $db->commitMode); + $this->assertSame(0, $this->getPrivateProperty($db, 'transDepth')); + $this->assertFalse($db->transStatus()); + } +} diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 75baa337d7b0..375d772e932f 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -36,6 +36,7 @@ Bugs Fixed - **CLI:** Fixed a bug where ``CLI::promptByMultipleKeys()`` accepted a key missing from the options when it was below the last key, returning an empty selection, rejected a key above the last one when the options were not in key order, and offered ``0`` as the default when the first key was not ``0``. - **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. See `#10648 `_. - **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::setDefaultSrc()`` where passing an array of URIs resulted in an invalid ``'Array'`` directive. diff --git a/user_guide_src/source/database/connecting.rst b/user_guide_src/source/database/connecting.rst index 65588dca4187..6099c7abc2a1 100644 --- a/user_guide_src/source/database/connecting.rst +++ b/user_guide_src/source/database/connecting.rst @@ -99,7 +99,14 @@ which can gracefully keep the connection alive or re-establish it. The ``reconnect()`` method pings the server to check if the connection is still alive. If the connection has been lost, it will close and re-establish the -connection. +connection. An open transaction ends with the old connection. ``reconnect()`` +clears the transaction depth and marks the transaction failed before it +opens the new connection, so ``transComplete()`` does not report success. +On OCI8, commit mode returns to autocommit. + +This only applies when ``ping()`` reports the connection lost. On PostgreSQL, +``ping()`` uses ``pg_ping()``, which re-establishes a broken connection itself +and returns true, so ``reconnect()`` does not clear the transaction state. .. literalinclude:: connecting/007.php :lines: 2-