Skip to content

Commit cfc8ac0

Browse files
fabpotfabpot
committed
bug symfony#32071 Fix expired lock not cleaned (jderusse)
This PR was merged into the 3.4 branch. Discussion ---------- Fix expired lock not cleaned | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#31426 | License | MIT | Doc PR | NA When a lock is acquired BUT not as fast as expected, a LockExpiredException is thrown. Issue is, that the lock is not removed which avoid other process to acquire that lock. This PR clean state of store when a LockExpiredException is triggered. note: same bug should be fixed in 4.3 in PDO and Zookeeper Commits ------- 9f960f3 Fix expired lock not cleaned
2 parents a8520ae + 9f960f3 commit cfc8ac0

File tree

6 files changed

+77
-18
lines changed

6 files changed

+77
-18
lines changed

‎src/Symfony/Component/Lock/Lock.php

+10
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ public function acquire($blocking = false)
8383
}
8484

8585
if ($this->key->isExpired()) {
86+
try {
87+
$this->release();
88+
} catch (\Exception $e) {
89+
// swallow exception to not hide the original issue
90+
}
8691
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $this->key));
8792
}
8893

@@ -117,6 +122,11 @@ public function refresh()
117122
$this->dirty = true;
118123

119124
if ($this->key->isExpired()) {
125+
try {
126+
$this->release();
127+
} catch (\Exception $e) {
128+
// swallow exception to not hide the original issue
129+
}
120130
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $this->key));
121131
}
122132

‎src/Symfony/Component/Lock/Store/CombinedStore.php

+4-4
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
use Psr\Log\NullLogger;
1717
use Symfony\Component\Lock\Exception\InvalidArgumentException;
1818
use Symfony\Component\Lock\Exception\LockConflictedException;
19-
use Symfony\Component\Lock\Exception\LockExpiredException;
2019
use Symfony\Component\Lock\Exception\NotSupportedException;
2120
use Symfony\Component\Lock\Key;
2221
use Symfony\Component\Lock\StoreInterface;
@@ -30,6 +29,7 @@
3029
class CombinedStore implements StoreInterface, LoggerAwareInterface
3130
{
3231
use LoggerAwareTrait;
32+
use ExpiringStoreTrait;
3333

3434
/** @var StoreInterface[] */
3535
private $stores;
@@ -78,6 +78,8 @@ public function save(Key $key)
7878
}
7979
}
8080

81+
$this->checkNotExpired($key);
82+
8183
if ($this->strategy->isMet($successCount, $storesCount)) {
8284
return;
8385
}
@@ -125,9 +127,7 @@ public function putOffExpiration(Key $key, $ttl)
125127
}
126128
}
127129

128-
if ($key->isExpired()) {
129-
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
130-
}
130+
$this->checkNotExpired($key);
131131

132132
if ($this->strategy->isMet($successCount, $storesCount)) {
133133
return;

‎src/Symfony/Component/Lock/Store/ExpiringStoreTrait.php

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Lock\Store;
13+
14+
use Symfony\Component\Lock\Exception\LockExpiredException;
15+
use Symfony\Component\Lock\Key;
16+
17+
trait ExpiringStoreTrait
18+
{
19+
private function checkNotExpired(Key $key)
20+
{
21+
if ($key->isExpired()) {
22+
try {
23+
$this->delete($key);
24+
} catch (\Exception $e) {
25+
// swallow exception to not hide the original issue
26+
}
27+
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
28+
}
29+
}
30+
}

‎src/Symfony/Component/Lock/Store/MemcachedStore.php

+4-7
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
use Symfony\Component\Lock\Exception\InvalidArgumentException;
1515
use Symfony\Component\Lock\Exception\LockConflictedException;
16-
use Symfony\Component\Lock\Exception\LockExpiredException;
1716
use Symfony\Component\Lock\Key;
1817
use Symfony\Component\Lock\StoreInterface;
1918

@@ -24,6 +23,8 @@
2423
*/
2524
class MemcachedStore implements StoreInterface
2625
{
26+
use ExpiringStoreTrait;
27+
2728
private $memcached;
2829
private $initialTtl;
2930
/** @var bool */
@@ -64,9 +65,7 @@ public function save(Key $key)
6465
$this->putOffExpiration($key, $this->initialTtl);
6566
}
6667

67-
if ($key->isExpired()) {
68-
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
69-
}
68+
$this->checkNotExpired($key);
7069
}
7170

7271
public function waitAndSave(Key $key)
@@ -110,9 +109,7 @@ public function putOffExpiration(Key $key, $ttl)
110109
throw new LockConflictedException();
111110
}
112111

113-
if ($key->isExpired()) {
114-
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
115-
}
112+
$this->checkNotExpired($key);
116113
}
117114

118115
/**

‎src/Symfony/Component/Lock/Store/RedisStore.php

+4-7
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
use Symfony\Component\Cache\Traits\RedisProxy;
1515
use Symfony\Component\Lock\Exception\InvalidArgumentException;
1616
use Symfony\Component\Lock\Exception\LockConflictedException;
17-
use Symfony\Component\Lock\Exception\LockExpiredException;
1817
use Symfony\Component\Lock\Key;
1918
use Symfony\Component\Lock\StoreInterface;
2019

@@ -25,6 +24,8 @@
2524
*/
2625
class RedisStore implements StoreInterface
2726
{
27+
use ExpiringStoreTrait;
28+
2829
private $redis;
2930
private $initialTtl;
3031

@@ -66,9 +67,7 @@ public function save(Key $key)
6667
throw new LockConflictedException();
6768
}
6869

69-
if ($key->isExpired()) {
70-
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
71-
}
70+
$this->checkNotExpired($key);
7271
}
7372

7473
public function waitAndSave(Key $key)
@@ -94,9 +93,7 @@ public function putOffExpiration(Key $key, $ttl)
9493
throw new LockConflictedException();
9594
}
9695

97-
if ($key->isExpired()) {
98-
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
99-
}
96+
$this->checkNotExpired($key);
10097
}
10198

10299
/**

‎src/Symfony/Component/Lock/Tests/Store/ExpiringStoreTestTrait.php

+25
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

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

14+
use Symfony\Component\Lock\Exception\LockExpiredException;
1415
use Symfony\Component\Lock\Key;
1516
use Symfony\Component\Lock\StoreInterface;
1617

@@ -105,4 +106,28 @@ public function testSetExpiration()
105106
$this->assertGreaterThanOrEqual(0, $key->getRemainingLifetime());
106107
$this->assertLessThanOrEqual(1, $key->getRemainingLifetime());
107108
}
109+
110+
public function testExpiredLockCleaned()
111+
{
112+
$resource = uniqid(__METHOD__, true);
113+
114+
$key1 = new Key($resource);
115+
$key2 = new Key($resource);
116+
117+
/** @var StoreInterface $store */
118+
$store = $this->getStore();
119+
$key1->reduceLifetime(0);
120+
121+
$this->assertTrue($key1->isExpired());
122+
try {
123+
$store->save($key1);
124+
$this->fail('The store shouldn\'t have save an expired key');
125+
} catch (LockExpiredException $e) {
126+
}
127+
128+
$this->assertFalse($store->exists($key1));
129+
130+
$store->save($key2);
131+
$this->assertTrue($store->exists($key2));
132+
}
108133
}

0 commit comments

Comments
 (0)