Skip to content

Commit de07372

Browse files
codercmscodercms
authored
Merge pull request #6 from makise-co/feature/transaction-stmt-error-fix
Bug fixes #2
2 parents 6e95585 + a0bef06 commit de07372

File tree

6 files changed

+241
-56
lines changed

6 files changed

+241
-56
lines changed

‎src/Driver/MakisePostgres/Bridge/PooledTransaction.php‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
use MakiseCo\Postgres\Contracts\Transaction;
1414
use Psr\Log\LoggerAwareTrait;
1515
use Psr\Log\LoggerInterface;
16+
use Throwable;
17+
18+
use function array_keys;
1619

1720
class PooledTransaction
1821
{
@@ -21,6 +24,14 @@ class PooledTransaction
2124
private Transaction $transaction;
2225
private int $level = 1;
2326

27+
/**
28+
* Statements list that should be deallocated when transaction error occurred
29+
* We cannot deallocate statements inside broken transaction
30+
*
31+
* @var \MakiseCo\SqlCommon\Contracts\Statement[]
32+
*/
33+
private array $statementsToDeallocate = [];
34+
2435
public function __construct(Transaction $transaction, ?LoggerInterface $logger = null)
2536
{
2637
$this->transaction = $transaction;
@@ -55,7 +66,7 @@ public function commit(): void
5566
try {
5667
$this->transaction->commit();
5768
return;
58-
} catch (\Throwable $e) {
69+
} catch (Throwable $e) {
5970
throw ExceptionMapper::map($e, 'COMMIT');
6071
} finally {
6172
$this->level = 0;
@@ -76,10 +87,21 @@ public function rollbackTransaction(): void
7687
$this->transaction->rollback();
7788

7889
return;
79-
} catch (\Throwable $e) {
90+
} catch (Throwable $e) {
8091
throw ExceptionMapper::map($e, 'ROLLBACK');
8192
} finally {
8293
$this->level = 0;
94+
95+
foreach (array_keys($this->statementsToDeallocate) as $key) {
96+
try {
97+
unset($this->statementsToDeallocate[$key]);
98+
} catch (Throwable $deallocEx) {
99+
$this->logger->notice(
100+
"Failed to deallocate statement on transaction rollback: {$deallocEx->getMessage()}"
101+
);
102+
}
103+
}
104+
$this->statementsToDeallocate = [];
83105
}
84106
}
85107

@@ -94,7 +116,7 @@ public function createSavepoint(int $level): void
94116

95117
try {
96118
$this->transaction->createSavepoint("SVP{$level}");
97-
} catch (\Throwable $e) {
119+
} catch (Throwable $e) {
98120
throw ExceptionMapper::map($e, "SAVEPOINT 'SVP{$level}'");
99121
}
100122

@@ -109,7 +131,7 @@ public function releaseSavepoint(int $level): void
109131

110132
try {
111133
$this->transaction->releaseSavepoint("SVP{$level}");
112-
} catch (\Throwable $e) {
134+
} catch (Throwable $e) {
113135
throw ExceptionMapper::map($e, "RELEASE SAVEPOINT 'SVP{$level}'");
114136
}
115137

@@ -124,10 +146,15 @@ public function rollbackSavepoint(int $level): void
124146

125147
try {
126148
$this->transaction->rollbackTo("SVP{$level}");
127-
} catch (\Throwable $e) {
149+
} catch (Throwable $e) {
128150
throw ExceptionMapper::map($e, "ROLLBACK TO SAVEPOINT 'SVP{$level}'");
129151
}
130152

131153
$this->level--;
132154
}
133-
}
155+
156+
public function addStatementToDeallocate(\MakiseCo\SqlCommon\Contracts\Statement $statement): void
157+
{
158+
$this->statementsToDeallocate[] = $statement;
159+
}
160+
}

‎src/Driver/MakisePostgres/Bridge/Statement.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ final class Statement implements StatementInterface, IteratorAggregate
4545
*/
4646
public function __construct(PostgresStatement $pdoStatement, $result)
4747
{
48-
$this->pdoStatement = $pdoStatement;
48+
// TODO: This preventing from infinite locking, statement should be release immediately
49+
// $this->pdoStatement = $pdoStatement;
4950
$this->queryString = $pdoStatement->getQuery();
5051

5152
if ($result instanceof ResultSet) {
@@ -76,6 +77,7 @@ public function getQueryString(): string
7677
*/
7778
public function getPDOStatement(): PostgresStatement
7879
{
80+
throw new \RuntimeException('This is not a PDO');
7981
return $this->pdoStatement;
8082
}
8183

‎src/Driver/MakisePostgres/PooledMakisePostgresDriver.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -532,12 +532,10 @@ protected function statement(
532532
return $this->statement($query, $parameters, false);
533533
}
534534

535-
// an exception occurred during transaction
536-
if ($this->getTransactionConn() !== null) {
537-
try {
538-
unset($statement);
539-
} catch (\Throwable $stmtCloseEx) {
540-
}
535+
if (($conn = $this->getTransactionConn()) !== null && isset($statement)) {
536+
// statements cannot be deallocated inside broken transaction block
537+
// so statements de-allocation should be delayed until transaction rollback
538+
$conn->addStatementToDeallocate($statement);
541539
}
542540

543541
throw $e;

‎tests/Database/Driver/MakisePostgresPool/ExtendedTransactionsTest.php‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Makise-Co Framework
5+
*
6+
* World line: 0.571024a
7+
* (c) Dmitry K. <[email protected]>
8+
*/
9+
10+
declare(strict_types=1);
11+
12+
namespace MakiseCo\Database\Tests\Driver\MakisePostgresPool;
13+
14+
use MakiseCo\Database\Driver\MakisePostgres\Bridge\PostgresPool;
15+
use MakiseCo\Database\Tests\BaseTest;
16+
use Spiral\Database\Database;
17+
use Spiral\Database\Exception\StatementException\ConstrainException;
18+
19+
class ExtendedTransactionsTest extends BaseTest
20+
{
21+
public const DRIVER = 'makisepostgrespool';
22+
23+
public function setUp(): void
24+
{
25+
$this->database = $this->db();
26+
27+
/** @var PostgresPool $pool */
28+
$pool= $this->database->getDriver()->getPool();
29+
$pool->setMaxActive(1);
30+
}
31+
32+
public function testConstraintExceptionOnTransaction(): void
33+
{
34+
$this->expectException(ConstrainException::class);
35+
36+
$this->database->transaction(
37+
function (Database $db) {
38+
$res = $db->query(
39+
<<<SQL
40+
CREATE TABLE uniq_table (
41+
id int8 PRIMARY KEY
42+
);
43+
SQL
44+
);
45+
$db->insert('uniq_table')->values(['id' => 1])->run();
46+
$db->insert('uniq_table')->values(['id' => 1])->run();
47+
}
48+
);
49+
}
50+
51+
public function testConstraintExceptionOnNestedTransaction(): void
52+
{
53+
$this->expectException(ConstrainException::class);
54+
55+
$this->database->transaction(
56+
function (Database $db) {
57+
$res = $db->query(
58+
<<<SQL
59+
CREATE TABLE uniq_table (
60+
id int8 PRIMARY KEY
61+
);
62+
SQL
63+
);
64+
$db->insert('uniq_table')->values(['id' => 1])->run();
65+
66+
$db->transaction(function (Database $db) {
67+
$db->insert('uniq_table')->values(['id' => 1])->run();
68+
});
69+
}
70+
);
71+
}
72+
73+
public function testConstraintExceptionOnMultipleTimesTransaction(): void
74+
{
75+
$this->expectException(ConstrainException::class);
76+
77+
try {
78+
$this->database->transaction(
79+
function (Database $db) {
80+
$res = $db->query(
81+
<<<SQL
82+
CREATE TABLE uniq_table (
83+
id int8 PRIMARY KEY
84+
);
85+
SQL
86+
);
87+
$db->insert('uniq_table')->values(['id' => 1])->run();
88+
89+
$db->transaction(
90+
function (Database $db) {
91+
$db->insert('uniq_table')->values(['id' => 1])->run();
92+
}
93+
);
94+
}
95+
);
96+
} catch (ConstrainException $e) {
97+
}
98+
99+
try {
100+
$this->database->transaction(
101+
function (Database $db) {
102+
$res = $db->query(
103+
<<<SQL
104+
CREATE TABLE uniq_table (
105+
id int8 PRIMARY KEY
106+
);
107+
SQL
108+
);
109+
$db->insert('uniq_table')->values(['id' => 1])->run();
110+
111+
$db->transaction(
112+
function (Database $db) {
113+
$db->insert('uniq_table')->values(['id' => 1])->run();
114+
}
115+
);
116+
}
117+
);
118+
} catch (ConstrainException $e) {
119+
}
120+
121+
$this->database->transaction(
122+
function (Database $db) {
123+
$res = $db->query(
124+
<<<SQL
125+
CREATE TABLE uniq_table (
126+
id int8 PRIMARY KEY
127+
);
128+
SQL
129+
);
130+
$db->insert('uniq_table')->values(['id' => 1])->run();
131+
132+
$db->transaction(
133+
function (Database $db) {
134+
$db->insert('uniq_table')->values(['id' => 1])->run();
135+
}
136+
);
137+
}
138+
);
139+
}
140+
}

‎tests/Database/Driver/MakisePostgresPool/TransactionsTest.php‎

Lines changed: 56 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -11,51 +11,68 @@
1111

1212
namespace MakiseCo\Database\Tests\Driver\MakisePostgresPool;
1313

14-
use Spiral\Database\Database;
15-
use Spiral\Database\Exception\StatementException\ConstrainException;
14+
use MakiseCo\Database\Driver\MakisePostgres\Bridge\PostgresPool;
15+
use Swoole\Coroutine;
1616

1717
class TransactionsTest extends \MakiseCo\Database\Tests\TransactionsTest
1818
{
1919
public const DRIVER = 'makisepostgrespool';
2020

21-
public function testConstraintExceptionOnTransaction(): void
21+
public function testConcurrentTransactions(): void
2222
{
23-
$this->expectException(ConstrainException::class);
24-
25-
$this->database->transaction(
26-
function (Database $db) {
27-
$res = $db->query(
28-
<<<SQL
29-
CREATE TABLE uniq_table (
30-
id int8 PRIMARY KEY
31-
);
32-
SQL
33-
);
34-
$db->insert('uniq_table')->values(['id' => 1])->run();
35-
$db->insert('uniq_table')->values(['id' => 1])->run();
36-
}
37-
);
38-
}
23+
$db = $this->database;
24+
/** @var PostgresPool $pool */
25+
$pool = $db->getDriver()->getPool();
3926

40-
public function testConstraintExceptionOnNestedTransaction(): void
41-
{
42-
$this->expectException(ConstrainException::class);
43-
44-
$this->database->transaction(
45-
function (Database $db) {
46-
$res = $db->query(
47-
<<<SQL
48-
CREATE TABLE uniq_table (
49-
id int8 PRIMARY KEY
50-
);
51-
SQL
52-
);
53-
$db->insert('uniq_table')->values(['id' => 1])->run();
54-
55-
$db->transaction(function (Database $db) {
56-
$db->insert('uniq_table')->values(['id' => 1])->run();
57-
});
58-
}
59-
);
27+
self::assertSame(2, $pool->getMaxActive());
28+
29+
$ch = new Coroutine\Channel(2);
30+
31+
Coroutine::create(function () use ($db, $ch) {
32+
$this->database->transaction(
33+
function () use ($db, $ch): void {
34+
try {
35+
$id = $db->table->insertOne(['name' => 'Anton', 'value' => 123]);
36+
$this->assertSame($id, $this->database->table->count());
37+
$db->query('SELECT pg_sleep(0.5)');
38+
39+
$ch->push($id);
40+
} catch (\Throwable $e) {
41+
$ch->push($e);
42+
}
43+
}
44+
);
45+
});
46+
47+
Coroutine::create(function () use ($db, $ch) {
48+
$this->database->transaction(
49+
function () use ($db, $ch): void {
50+
try {
51+
$id = $db->table->insertOne(['name' => 'Dmitry', 'value' => 456]);
52+
$this->assertSame(1, $this->database->table->count());
53+
$db->query('SELECT pg_sleep(0.5)');
54+
55+
$ch->push($id);
56+
} catch (\Throwable $e) {
57+
$ch->push($e);
58+
}
59+
}
60+
);
61+
});
62+
63+
if (($err = $ch->pop()) instanceof \Throwable) {
64+
throw $err;
65+
}
66+
$ids[] = $err;
67+
68+
if (($err = $ch->pop()) instanceof \Throwable) {
69+
throw $err;
70+
}
71+
$ids[] = $err;
72+
73+
self::assertSame(2, $this->database->table->count());
74+
self::assertSame(2, $pool->getIdleCount());
75+
self::assertContains(1, $ids);
76+
self::assertContains(2, $ids);
6077
}
6178
}

‎tests/Database/StatementTest.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,11 @@ public function testInstance(): void
7070
$table->select()->getIterator()
7171
);
7272

73-
$this->assertInstanceOf(
74-
Statement::class,
75-
$table->select()->run()->getPDOStatement()
76-
);
73+
// We do not storing PDO statement anymore
74+
// $this->assertInstanceOf(
75+
// Statement::class,
76+
// $table->select()->run()->getPDOStatement()
77+
// );
7778
}
7879

7980
//We are testing only extended functionality, there is no need to test PDOStatement

0 commit comments

Comments
 (0)