Skip to content

Commit 0cf4610

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '5.4' into 6.0
* 5.4: [Lock] Release Locks from Internal Store on Postgres waitAndSave* [Serializer] Fix passing null to str_contains() [DependencyInjection] Don't reset env placeholders during compilation [HttpClient] Fix overriding default options with null [DependencyInjection] Clarify that using expressions in parameters is not allowed [GHA] Cancel running CI jobs when a PR is updated [Validator] Improve tests for the Image and File constraints [Validator][Tests] Fix AssertingContextualValidator not throwing on remaining expectations
2 parents 4aab26e + f03b561 commit 0cf4610

File tree

4 files changed

+215
-21
lines changed

4 files changed

+215
-21
lines changed

‎Store/DoctrineDbalPostgreSqlStore.php

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -169,10 +169,18 @@ public function waitAndSave(Key $key)
169169
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
170170
$this->getInternalStore()->save($key);
171171

172+
$lockAcquired = false;
172173
$sql = 'SELECT pg_advisory_lock(:key)';
173-
$this->conn->executeStatement($sql, [
174-
'key' => $this->getHashedKey($key),
175-
]);
174+
try {
175+
$this->conn->executeStatement($sql, [
176+
'key' => $this->getHashedKey($key),
177+
]);
178+
$lockAcquired = true;
179+
} finally {
180+
if (!$lockAcquired) {
181+
$this->getInternalStore()->delete($key);
182+
}
183+
}
176184

177185
// release lock in case of promotion
178186
$this->unlockShared($key);
@@ -184,10 +192,18 @@ public function waitAndSaveRead(Key $key)
184192
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
185193
$this->getInternalStore()->saveRead($key);
186194

195+
$lockAcquired = false;
187196
$sql = 'SELECT pg_advisory_lock_shared(:key)';
188-
$this->conn->executeStatement($sql, [
189-
'key' => $this->getHashedKey($key),
190-
]);
197+
try {
198+
$this->conn->executeStatement($sql, [
199+
'key' => $this->getHashedKey($key),
200+
]);
201+
$lockAcquired = true;
202+
} finally {
203+
if (!$lockAcquired) {
204+
$this->getInternalStore()->delete($key);
205+
}
206+
}
191207

192208
// release lock in case of demotion
193209
$this->unlock($key);

‎Store/PostgreSqlStore.php

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,18 @@ public function waitAndSave(Key $key)
185185
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
186186
$this->getInternalStore()->save($key);
187187

188+
$lockAcquired = false;
188189
$sql = 'SELECT pg_advisory_lock(:key)';
189-
$stmt = $this->getConnection()->prepare($sql);
190-
191-
$stmt->bindValue(':key', $this->getHashedKey($key));
192-
$stmt->execute();
190+
try {
191+
$stmt = $this->getConnection()->prepare($sql);
192+
$stmt->bindValue(':key', $this->getHashedKey($key));
193+
$stmt->execute();
194+
$lockAcquired = true;
195+
} finally {
196+
if (!$lockAcquired) {
197+
$this->getInternalStore()->delete($key);
198+
}
199+
}
193200

194201
// release lock in case of promotion
195202
$this->unlockShared($key);
@@ -201,11 +208,19 @@ public function waitAndSaveRead(Key $key)
201208
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
202209
$this->getInternalStore()->saveRead($key);
203210

211+
$lockAcquired = false;
204212
$sql = 'SELECT pg_advisory_lock_shared(:key)';
205-
$stmt = $this->getConnection()->prepare($sql);
206213

207-
$stmt->bindValue(':key', $this->getHashedKey($key));
208-
$stmt->execute();
214+
try {
215+
$stmt = $this->getConnection()->prepare($sql);
216+
$stmt->bindValue(':key', $this->getHashedKey($key));
217+
$stmt->execute();
218+
$lockAcquired = true;
219+
} finally {
220+
if (!$lockAcquired) {
221+
$this->getInternalStore()->delete($key);
222+
}
223+
}
209224

210225
// release lock in case of demotion
211226
$this->unlock($key);

‎Tests/Store/DoctrineDbalPostgreSqlStoreTest.php

Lines changed: 84 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111

1212
namespace Symfony\Component\Lock\Tests\Store;
1313

14+
use Doctrine\DBAL\Connection;
1415
use Doctrine\DBAL\DriverManager;
16+
use Doctrine\DBAL\Exception as DBALException;
1517
use Symfony\Component\Lock\Exception\InvalidArgumentException;
1618
use Symfony\Component\Lock\Exception\LockConflictedException;
1719
use Symfony\Component\Lock\Key;
@@ -29,15 +31,21 @@ class DoctrineDbalPostgreSqlStoreTest extends AbstractStoreTest
2931
use BlockingStoreTestTrait;
3032
use SharedLockStoreTestTrait;
3133

34+
public function createPostgreSqlConnection(): Connection
35+
{
36+
if (!getenv('POSTGRES_HOST')) {
37+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
38+
}
39+
40+
return DriverManager::getConnection(['url' => 'pgsql://postgres:password@'.getenv('POSTGRES_HOST')]);
41+
}
42+
3243
/**
3344
* {@inheritdoc}
3445
*/
3546
public function getStore(): PersistingStoreInterface
3647
{
37-
if (!getenv('POSTGRES_HOST')) {
38-
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
39-
}
40-
$conn = DriverManager::getConnection(['url' => 'pgsql://postgres:password@'.getenv('POSTGRES_HOST')]);
48+
$conn = $this->createPostgreSqlConnection();
4149

4250
return new DoctrineDbalPostgreSqlStore($conn);
4351
}
@@ -86,4 +94,76 @@ public function testSaveAfterConflict()
8694
$store2->save($key);
8795
$this->assertTrue($store2->exists($key));
8896
}
97+
98+
public function testWaitAndSaveAfterConflictReleasesLockFromInternalStore()
99+
{
100+
$store1 = $this->getStore();
101+
$conn = $this->createPostgreSqlConnection();
102+
$store2 = new DoctrineDbalPostgreSqlStore($conn);
103+
104+
$keyId = uniqid(__METHOD__, true);
105+
$store1Key = new Key($keyId);
106+
107+
$store1->save($store1Key);
108+
109+
// set a low time out then try to wait and save, which will fail
110+
// because the key is already set above.
111+
$conn->executeStatement('SET statement_timeout = 1');
112+
$waitSaveError = null;
113+
try {
114+
$store2->waitAndSave(new Key($keyId));
115+
} catch (DBALException $waitSaveError) {
116+
}
117+
$this->assertInstanceOf(DBALException::class, $waitSaveError, 'waitAndSave should have thrown');
118+
$conn->executeStatement('SET statement_timeout = 0');
119+
120+
$store1->delete($store1Key);
121+
$this->assertFalse($store1->exists($store1Key));
122+
123+
$store2Key = new Key($keyId);
124+
$lockConflicted = false;
125+
try {
126+
$store2->waitAndSave($store2Key);
127+
} catch (LockConflictedException $lockConflictedException) {
128+
$lockConflicted = true;
129+
}
130+
131+
$this->assertFalse($lockConflicted, 'lock should be available now that its been remove from $store1');
132+
$this->assertTrue($store2->exists($store2Key));
133+
}
134+
135+
public function testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore()
136+
{
137+
$store1 = $this->getStore();
138+
$conn = $this->createPostgreSqlConnection();
139+
$store2 = new DoctrineDbalPostgreSqlStore($conn);
140+
141+
$keyId = uniqid(__METHOD__, true);
142+
$store1Key = new Key($keyId);
143+
144+
$store1->save($store1Key);
145+
146+
// set a low time out then try to wait and save, which will fail
147+
// because the key is already set above.
148+
$conn->executeStatement('SET statement_timeout = 1');
149+
$waitSaveError = null;
150+
try {
151+
$store2->waitAndSaveRead(new Key($keyId));
152+
} catch (DBALException $waitSaveError) {
153+
}
154+
$this->assertInstanceOf(DBALException::class, $waitSaveError, 'waitAndSaveRead should have thrown');
155+
156+
$store1->delete($store1Key);
157+
$this->assertFalse($store1->exists($store1Key));
158+
159+
$store2Key = new Key($keyId);
160+
// since the lock is going to be acquired in read mode and is not exclusive
161+
// this won't every throw a LockConflictedException as it would from
162+
// waitAndSave, but it will hang indefinitely as it waits for postgres
163+
// so set a time out of 2 seconds here so the test doesn't just sit forever
164+
$conn->executeStatement('SET statement_timeout = 2000');
165+
$store2->waitAndSaveRead($store2Key);
166+
167+
$this->assertTrue($store2->exists($store2Key));
168+
}
89169
}

‎Tests/Store/PostgreSqlStoreTest.php

Lines changed: 87 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,23 @@ class PostgreSqlStoreTest extends AbstractStoreTest
2828
use BlockingStoreTestTrait;
2929
use SharedLockStoreTestTrait;
3030

31+
public function getPostgresHost(): string
32+
{
33+
if (!$host = getenv('POSTGRES_HOST')) {
34+
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
35+
}
36+
37+
return $host;
38+
}
39+
3140
/**
3241
* {@inheritdoc}
3342
*/
3443
public function getStore(): PersistingStoreInterface
3544
{
36-
if (!getenv('POSTGRES_HOST')) {
37-
$this->markTestSkipped('Missing POSTGRES_HOST env variable');
38-
}
45+
$host = $this->getPostgresHost();
3946

40-
return new PostgreSqlStore('pgsql:host='.getenv('POSTGRES_HOST'), ['db_username' => 'postgres', 'db_password' => 'password']);
47+
return new PostgreSqlStore('pgsql:host='.$host, ['db_username' => 'postgres', 'db_password' => 'password']);
4148
}
4249

4350
/**
@@ -78,4 +85,80 @@ public function testSaveAfterConflict()
7885
$store2->save($key);
7986
$this->assertTrue($store2->exists($key));
8087
}
88+
89+
public function testWaitAndSaveAfterConflictReleasesLockFromInternalStore()
90+
{
91+
$store1 = $this->getStore();
92+
$postgresHost = $this->getPostgresHost();
93+
$pdo = new \PDO('pgsql:host='.$postgresHost, 'postgres', 'password');
94+
$pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
95+
$store2 = new PostgreSqlStore($pdo);
96+
97+
$keyId = uniqid(__METHOD__, true);
98+
$store1Key = new Key($keyId);
99+
100+
$store1->save($store1Key);
101+
102+
// set a low time out then try to wait and save, which will fail
103+
// because the key is already set above.
104+
$pdo->exec('SET statement_timeout = 1');
105+
$waitSaveError = null;
106+
try {
107+
$store2->waitAndSave(new Key($keyId));
108+
} catch (\PDOException $waitSaveError) {
109+
}
110+
$this->assertInstanceOf(\PDOException::class, $waitSaveError, 'waitAndSave should have thrown');
111+
$pdo->exec('SET statement_timeout = 0');
112+
113+
$store1->delete($store1Key);
114+
$this->assertFalse($store1->exists($store1Key));
115+
116+
$store2Key = new Key($keyId);
117+
$lockConflicted = false;
118+
try {
119+
$store2->waitAndSave($store2Key);
120+
} catch (LockConflictedException $lockConflictedException) {
121+
$lockConflicted = true;
122+
}
123+
124+
$this->assertFalse($lockConflicted, 'lock should be available now that its been remove from $store1');
125+
$this->assertTrue($store2->exists($store2Key));
126+
}
127+
128+
public function testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore()
129+
{
130+
$store1 = $this->getStore();
131+
$postgresHost = $this->getPostgresHost();
132+
$pdo = new \PDO('pgsql:host='.$postgresHost, 'postgres', 'password');
133+
$pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
134+
$store2 = new PostgreSqlStore($pdo);
135+
136+
$keyId = uniqid(__METHOD__, true);
137+
$store1Key = new Key($keyId);
138+
139+
$store1->save($store1Key);
140+
141+
// set a low time out then try to wait and save, which will fail
142+
// because the key is already set above.
143+
$pdo->exec('SET statement_timeout = 1');
144+
$waitSaveError = null;
145+
try {
146+
$store2->waitAndSaveRead(new Key($keyId));
147+
} catch (\PDOException $waitSaveError) {
148+
}
149+
$this->assertInstanceOf(\PDOException::class, $waitSaveError, 'waitAndSave should have thrown');
150+
151+
$store1->delete($store1Key);
152+
$this->assertFalse($store1->exists($store1Key));
153+
154+
$store2Key = new Key($keyId);
155+
// since the lock is going to be acquired in read mode and is not exclusive
156+
// this won't every throw a LockConflictedException as it would from
157+
// waitAndSave, but it will hang indefinitely as it waits for postgres
158+
// so set a time out of 2 seconds here so the test doesn't just sit forever
159+
$pdo->exec('SET statement_timeout = 20000');
160+
$store2->waitAndSaveRead($store2Key);
161+
162+
$this->assertTrue($store2->exists($store2Key));
163+
}
81164
}

0 commit comments

Comments
 (0)