Skip to content

Commit 8ab9933

Browse files
KevinBlakeLopezKevinBlakeLopez
authored
fix: clear transaction state when reconnect() replaces the socket (#10650)
1 parent f992c27 commit 8ab9933

6 files changed

Lines changed: 259 additions & 1 deletion

File tree

‎system/Database/BaseConnection.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -625,16 +625,40 @@ public function close()
625625
* Keep or establish the connection if no queries have been sent for
626626
* a length of time exceeding the server's idle timeout.
627627
*
628+
* A replaced socket is not in the transaction that was open on the old
629+
* one. The server has already rolled that work back. Clear the depth and
630+
* mark the transaction failed before opening the new connection, so a
631+
* failed reconnect cannot still report success.
632+
*
628633
* @return void
629634
*/
630635
public function reconnect()
631636
{
632637
if ($this->ping() === false) {
638+
$lostTransaction = $this->transDepth > 0;
639+
633640
$this->close();
641+
642+
if ($lostTransaction) {
643+
$this->abandonLostTransaction();
644+
}
645+
634646
$this->initialize();
635647
}
636648
}
637649

650+
/**
651+
* The socket that owned the transaction is already closed.
652+
*
653+
* Drivers that keep transaction state in PHP, rather than only on the
654+
* server, must override this.
655+
*/
656+
protected function abandonLostTransaction(): void
657+
{
658+
$this->transDepth = 0;
659+
$this->transStatus = false;
660+
}
661+
638662
/**
639663
* Platform dependent way method for closing the connection.
640664
*

‎system/Database/OCI8/Connection.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,17 @@ protected function _transRollback(): bool
722722
return oci_rollback($this->connID);
723723
}
724724

725+
/**
726+
* Restore OCI_COMMIT_ON_SUCCESS. close() does not, and
727+
* OCI_NO_AUTO_COMMIT would leave later writes uncommitted.
728+
*/
729+
protected function abandonLostTransaction(): void
730+
{
731+
$this->commitMode = OCI_COMMIT_ON_SUCCESS;
732+
733+
parent::abandonLostTransaction();
734+
}
735+
725736
/**
726737
* Returns the name of the current database being used.
727738
*/

‎tests/system/Database/Live/ReconnectDuringTransactionTest.php‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* This file is part of CodeIgniter 4 framework.
7+
*
8+
* (c) CodeIgniter Foundation <admin@codeigniter.com>
9+
*
10+
* For the full copyright and license information, please view
11+
* the LICENSE file that was distributed with this source code.
12+
*/
13+
14+
namespace CodeIgniter\Database\Live;
15+
16+
use CodeIgniter\Test\CIUnitTestCase;
17+
use CodeIgniter\Test\DatabaseTestTrait;
18+
use Config\Database;
19+
use PHPUnit\Framework\Attributes\Group;
20+
21+
/**
22+
* @internal
23+
*/
24+
#[Group('DatabaseLive')]
25+
final class ReconnectDuringTransactionTest extends CIUnitTestCase
26+
{
27+
use DatabaseTestTrait;
28+
29+
protected $migrate = false;
30+
31+
protected function setUp(): void
32+
{
33+
// Dedicated connection so a forced wait_timeout cannot drop the shared one.
34+
$this->db = Database::connect($this->DBGroup, false);
35+
36+
parent::setUp();
37+
}
38+
39+
public function testReconnectInsideTransactionDoesNotReportSuccess(): void
40+
{
41+
if ($this->db->DBDriver !== 'MySQLi') {
42+
$this->markTestSkipped('Requires the MySQLi driver.');
43+
}
44+
45+
$table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe'));
46+
$this->db->query('DROP TABLE IF EXISTS ' . $table);
47+
$this->db->query(
48+
'CREATE TABLE ' . $table . ' ('
49+
. 'id INT NOT NULL AUTO_INCREMENT, '
50+
. 'note VARCHAR(32) NOT NULL, '
51+
. 'PRIMARY KEY (id)'
52+
. ') ENGINE=InnoDB',
53+
);
54+
55+
$observer = Database::connect($this->DBGroup, false);
56+
$observer->initialize();
57+
58+
try {
59+
$this->db->transStart();
60+
$this->db->table('reconnect_probe')->insert(['note' => 'before-timeout']);
61+
$this->assertSame(1, $this->db->transDepth);
62+
$this->assertTrue($this->db->transStatus());
63+
64+
$this->db->query('SET SESSION wait_timeout = 1');
65+
sleep(3);
66+
67+
$this->assertFalse($this->db->ping(), 'Precondition: the server should have closed the idle socket.');
68+
69+
$this->db->reconnect();
70+
71+
$depthAfterReconnect = $this->db->transDepth;
72+
$statusAfterReconnect = $this->db->transStatus();
73+
$completed = $this->db->transComplete();
74+
$rows = (int) $observer->table('reconnect_probe')->countAllResults();
75+
76+
$this->assertSame(
77+
['depth' => 0, 'status' => false, 'completed' => false, 'rows' => 0],
78+
[
79+
'depth' => $depthAfterReconnect,
80+
'status' => $statusAfterReconnect,
81+
'completed' => $completed,
82+
'rows' => $rows,
83+
],
84+
);
85+
} finally {
86+
if ($this->db->transDepth > 0) {
87+
$this->db->transRollback();
88+
}
89+
90+
$this->db->query('DROP TABLE IF EXISTS ' . $table);
91+
$observer->close();
92+
$this->db->close();
93+
}
94+
}
95+
96+
public function testReconnectKeepsAnOpenTransactionWhenTheSocketIsAlive(): void
97+
{
98+
if ($this->db->DBDriver !== 'MySQLi') {
99+
$this->markTestSkipped('Requires the MySQLi driver.');
100+
}
101+
102+
$table = $this->db->escapeIdentifier($this->db->prefixTable('reconnect_probe'));
103+
$this->db->query('DROP TABLE IF EXISTS ' . $table);
104+
$this->db->query(
105+
'CREATE TABLE ' . $table . ' ('
106+
. 'id INT NOT NULL AUTO_INCREMENT, '
107+
. 'note VARCHAR(32) NOT NULL, '
108+
. 'PRIMARY KEY (id)'
109+
. ') ENGINE=InnoDB',
110+
);
111+
112+
try {
113+
$this->db->transStart();
114+
$this->db->table('reconnect_probe')->insert(['note' => 'still-open']);
115+
$this->assertTrue($this->db->ping());
116+
117+
$this->db->reconnect();
118+
119+
$this->assertSame(1, $this->db->transDepth);
120+
$this->assertTrue($this->db->transStatus());
121+
$this->assertTrue($this->db->transComplete());
122+
$this->assertSame(1, $this->db->table('reconnect_probe')->countAllResults());
123+
} finally {
124+
if ($this->db->transDepth > 0) {
125+
$this->db->transRollback();
126+
}
127+
128+
$this->db->query('DROP TABLE IF EXISTS ' . $table);
129+
$this->db->close();
130+
}
131+
}
132+
}

‎tests/system/Database/ReconnectLostTransactionTest.php‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* This file is part of CodeIgniter 4 framework.
7+
*
8+
* (c) CodeIgniter Foundation <admin@codeigniter.com>
9+
*
10+
* For the full copyright and license information, please view
11+
* the LICENSE file that was distributed with this source code.
12+
*/
13+
14+
namespace CodeIgniter\Database;
15+
16+
use CodeIgniter\Database\Exceptions\DatabaseException;
17+
use CodeIgniter\Database\MySQLi\Connection as MySQLiConnection;
18+
use CodeIgniter\Database\OCI8\Connection as OCI8Connection;
19+
use CodeIgniter\Test\CIUnitTestCase;
20+
use PHPUnit\Framework\Attributes\Group;
21+
22+
/**
23+
* @internal
24+
*/
25+
#[Group('Others')]
26+
final class ReconnectLostTransactionTest extends CIUnitTestCase
27+
{
28+
public function testReconnectClearsTransactionStateWhenInitializeFails(): void
29+
{
30+
$db = new class (['DBDriver' => 'MySQLi']) extends MySQLiConnection {
31+
public function initialize(): never
32+
{
33+
throw new DatabaseException('database unavailable');
34+
}
35+
};
36+
37+
$this->setPrivateProperty($db, 'transDepth', 1);
38+
39+
try {
40+
$db->reconnect();
41+
$this->fail('initialize() should have thrown.');
42+
} catch (DatabaseException $exception) {
43+
$this->assertSame('database unavailable', $exception->getMessage());
44+
}
45+
46+
$this->assertSame(0, $this->getPrivateProperty($db, 'transDepth'));
47+
$this->assertFalse($db->transStatus());
48+
}
49+
50+
public function testReconnectRestoresOci8CommitModeWhenTransactionIsLost(): void
51+
{
52+
if (! defined('OCI_COMMIT_ON_SUCCESS')) {
53+
define('OCI_COMMIT_ON_SUCCESS', 32);
54+
}
55+
56+
if (! defined('OCI_NO_AUTO_COMMIT')) {
57+
define('OCI_NO_AUTO_COMMIT', 0);
58+
}
59+
60+
$db = new class (['DBDriver' => 'OCI8']) extends OCI8Connection {
61+
public function initialize(): void
62+
{
63+
}
64+
};
65+
66+
$db->commitMode = OCI_NO_AUTO_COMMIT;
67+
$this->setPrivateProperty($db, 'transDepth', 1);
68+
69+
$db->reconnect();
70+
71+
$this->assertSame(OCI_COMMIT_ON_SUCCESS, $db->commitMode);
72+
$this->assertSame(0, $this->getPrivateProperty($db, 'transDepth'));
73+
$this->assertFalse($db->transStatus());
74+
}
75+
}

‎user_guide_src/source/changelogs/v4.7.6.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ Bugs Fixed
3838
- **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``.
3939
- **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.
4040
- **Commands:** Fixed a bug where generator commands did not sort the imports of a template with CRLF line endings.
41+
- **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.
4142
- **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``.
4243
- **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.
4344
- **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.

‎user_guide_src/source/database/connecting.rst‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,22 @@ which can gracefully keep the connection alive or re-establish it.
9999

100100
The ``reconnect()`` method pings the server to check if the connection is still
101101
alive. If the connection has been lost, it will close and re-establish the
102-
connection.
102+
connection. An open transaction ends with the old connection. ``reconnect()``
103+
clears the transaction depth and marks the transaction failed before it
104+
opens the new connection, so ``transComplete()`` does not report success.
105+
On OCI8, commit mode returns to autocommit.
106+
107+
The transaction-state reset described above only occurs when ``ping()``
108+
returns false. On PostgreSQL, ``ping()`` uses ``pg_ping()``, which can
109+
re-establish a broken connection and return true. In that case,
110+
``reconnect()`` does not clear the transaction state.
111+
112+
The lost transaction is not resumed on the new connection. Subsequent
113+
writes may be committed automatically and cannot be rolled back by
114+
calling ``transComplete()``. If the operation must remain atomic, stop
115+
executing its remaining queries. Handle the failure at the application
116+
level that owns the entire transaction, including any nested transaction
117+
groups, before deciding whether to retry the entire operation.
103118

104119
.. literalinclude:: connecting/007.php
105120
:lines: 2-

0 commit comments

Comments
 (0)