Skip to content

Commit f03b561

Browse files
chrisguitarguynicolas-grekas
chrisguitarguy
authored and
nicolas-grekas
committed
[Lock] Release Locks from Internal Store on Postgres waitAndSave*
1 parent ddbeb2e commit f03b561

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
@@ -173,10 +173,18 @@ public function waitAndSave(Key $key)
173173
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
174174
$this->getInternalStore()->save($key);
175175

176+
$lockAcquired = false;
176177
$sql = 'SELECT pg_advisory_lock(:key)';
177-
$this->conn->executeStatement($sql, [
178-
'key' => $this->getHashedKey($key),
179-
]);
178+
try {
179+
$this->conn->executeStatement($sql, [
180+
'key' => $this->getHashedKey($key),
181+
]);
182+
$lockAcquired = true;
183+
} finally {
184+
if (!$lockAcquired) {
185+
$this->getInternalStore()->delete($key);
186+
}
187+
}
180188

181189
// release lock in case of promotion
182190
$this->unlockShared($key);
@@ -188,10 +196,18 @@ public function waitAndSaveRead(Key $key)
188196
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
189197
$this->getInternalStore()->saveRead($key);
190198

199+
$lockAcquired = false;
191200
$sql = 'SELECT pg_advisory_lock_shared(:key)';
192-
$this->conn->executeStatement($sql, [
193-
'key' => $this->getHashedKey($key),
194-
]);
201+
try {
202+
$this->conn->executeStatement($sql, [
203+
'key' => $this->getHashedKey($key),
204+
]);
205+
$lockAcquired = true;
206+
} finally {
207+
if (!$lockAcquired) {
208+
$this->getInternalStore()->delete($key);
209+
}
210+
}
195211

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

‎Store/PostgreSqlStore.php‎

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

238+
$lockAcquired = false;
238239
$sql = 'SELECT pg_advisory_lock(:key)';
239-
$stmt = $this->getConnection()->prepare($sql);
240-
241-
$stmt->bindValue(':key', $this->getHashedKey($key));
242-
$stmt->execute();
240+
try {
241+
$stmt = $this->getConnection()->prepare($sql);
242+
$stmt->bindValue(':key', $this->getHashedKey($key));
243+
$stmt->execute();
244+
$lockAcquired = true;
245+
} finally {
246+
if (!$lockAcquired) {
247+
$this->getInternalStore()->delete($key);
248+
}
249+
}
243250

244251
// release lock in case of promotion
245252
$this->unlockShared($key);
@@ -257,11 +264,19 @@ public function waitAndSaveRead(Key $key)
257264
// Internal store does not allow blocking mode, because there is no way to acquire one in a single process
258265
$this->getInternalStore()->saveRead($key);
259266

267+
$lockAcquired = false;
260268
$sql = 'SELECT pg_advisory_lock_shared(:key)';
261-
$stmt = $this->getConnection()->prepare($sql);
262269

263-
$stmt->bindValue(':key', $this->getHashedKey($key));
264-
$stmt->execute();
270+
try {
271+
$stmt = $this->getConnection()->prepare($sql);
272+
$stmt->bindValue(':key', $this->getHashedKey($key));
273+
$stmt->execute();
274+
$lockAcquired = true;
275+
} finally {
276+
if (!$lockAcquired) {
277+
$this->getInternalStore()->delete($key);
278+
}
279+
}
265280

266281
// release lock in case of demotion
267282
$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)