Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restore shared lock ttl to previous value when releasing #37469

Merged
merged 2 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 76 additions & 2 deletions lib/private/Lock/MemcacheLockingProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,42 @@
*/
namespace OC\Lock;

use OCP\AppFramework\Utility\ITimeFactory;
use OCP\IMemcache;
use OCP\IMemcacheTTL;
use OCP\Lock\LockedException;

class MemcacheLockingProvider extends AbstractLockingProvider {
/** @var array<string, array{time: int, ttl: int}> */
private array $oldTTLs = [];

public function __construct(
private IMemcache $memcache,
private ITimeFactory $timeFactory,
int $ttl = 3600,
) {
parent::__construct($ttl);
}

private function setTTL(string $path): void {
private function setTTL(string $path, int $ttl = null, ?int $compare = null): void {
if (is_null($ttl)) {
$ttl = $this->ttl;
}
if ($this->memcache instanceof IMemcacheTTL) {
if ($compare !== null) {
$this->memcache->compareSetTTL($path, $compare, $ttl);
} else {
$this->memcache->setTTL($path, $ttl);
}
}
}

private function getTTL(string $path): int {
if ($this->memcache instanceof IMemcacheTTL) {
$this->memcache->setTTL($path, $this->ttl);
$ttl = $this->memcache->getTTL($path);
return $ttl === false ? -1 : $ttl;
} else {
return -1;
}
}

Expand All @@ -58,14 +79,22 @@ public function isLocked(string $path, int $type): bool {

public function acquireLock(string $path, int $type, ?string $readablePath = null): void {
if ($type === self::LOCK_SHARED) {
// save the old TTL to for `restoreTTL`
$this->oldTTLs[$path] = [
"ttl" => $this->getTTL($path),
"time" => $this->timeFactory->getTime()
];
if (!$this->memcache->inc($path)) {
throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath);
}
} else {
// when getting exclusive locks, we know there are no old TTLs to restore
icewind1991 marked this conversation as resolved.
Show resolved Hide resolved
$this->memcache->add($path, 0);
// ttl is updated automatically when the `set` succeeds
if (!$this->memcache->cas($path, 0, 'exclusive')) {
throw new LockedException($path, null, $this->getExistingLockForException($path), $readablePath);
}
unset($this->oldTTLs[$path]);
}
$this->setTTL($path);
$this->markAcquire($path, $type);
Expand All @@ -88,6 +117,12 @@ public function releaseLock(string $path, int $type): void {
$newValue = $this->memcache->dec($path);
}

if ($newValue > 0) {
$this->restoreTTL($path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the restoreTTL from changeLock not enough, given the scenario you described?
What is the rationale behind this one in releaseLock? For other kind of exceptions/failures?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still want to restore the ttl when a request just acquires + releases.

} else {
unset($this->oldTTLs[$path]);
}

// if we somehow release more locks then exists, reset the lock
if ($newValue < 0) {
$this->memcache->cad($path, $newValue);
Expand All @@ -106,13 +141,52 @@ public function changeLock(string $path, int $targetType): void {
} elseif ($targetType === self::LOCK_EXCLUSIVE) {
// we can only change a shared lock to an exclusive if there's only a single owner of the shared lock
if (!$this->memcache->cas($path, 1, 'exclusive')) {
$this->restoreTTL($path);
throw new LockedException($path, null, $this->getExistingLockForException($path));
}
unset($this->oldTTLs[$path]);
}
$this->setTTL($path);
$this->markChange($path, $targetType);
}

/**
* With shared locks, each time the lock is acquired, the ttl for the path is reset.
*
* Due to this "ttl extension" when a shared lock isn't freed correctly for any reason
* the lock won't expire until no shared locks are required for the path for 1h.
* This can lead to a client repeatedly trying to upload a file, and failing forever
* because the lock never gets the opportunity to expire.
*
* To help the lock expire in this case, we lower the TTL back to what it was before we
* took the shared lock *only* if nobody else got a shared lock after we did.
*
* This doesn't handle all cases where multiple requests are acquiring shared locks
* but it should handle some of the more common ones and not hurt things further
*/
private function restoreTTL(string $path): void {
if (isset($this->oldTTLs[$path])) {
$saved = $this->oldTTLs[$path];
$elapsed = $this->timeFactory->getTime() - $saved['time'];

// old value to compare to when setting ttl in case someone else changes the lock in the middle of this function
$value = $this->memcache->get($path);

$currentTtl = $this->getTTL($path);

// what the old ttl would be given the time elapsed since we acquired the lock
// note that if this gets negative the key will be expired directly when we set the ttl
$remainingOldTtl = $saved['ttl'] - $elapsed;
icewind1991 marked this conversation as resolved.
Show resolved Hide resolved
// what the currently ttl would be if nobody else acquired a lock since we did (+1 to cover rounding errors)
$expectedTtl = $this->ttl - $elapsed + 1;

// check if another request has acquired a lock (and didn't release it yet)
if ($currentTtl <= $expectedTtl) {
$this->setTTL($path, $remainingOldTtl, $value);
}
}
}

private function getExistingLockForException(string $path): string {
$existing = $this->memcache->get($path);
if (!$existing) {
Expand Down
10 changes: 9 additions & 1 deletion lib/private/Memcache/LoggerWrapperCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,18 @@ public function cad($key, $old) {
}

/** @inheritDoc */
public function setTTL($key, $ttl) {
public function setTTL(string $key, int $ttl) {
$this->wrappedCache->setTTL($key, $ttl);
}

public function getTTL(string $key): int|false {
return $this->wrappedCache->getTTL($key);
}

public function compareSetTTL(string $key, mixed $value, int $ttl): bool {
Fixed Show fixed Hide fixed
return $this->wrappedCache->compareSetTTL($key, $value, $ttl);
}

public static function isAvailable(): bool {
return true;
}
Expand Down
10 changes: 9 additions & 1 deletion lib/private/Memcache/ProfilerWrapperCache.php
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,18 @@ public function cad($key, $old) {
}

/** @inheritDoc */
public function setTTL($key, $ttl) {
public function setTTL(string $key, int $ttl) {
$this->wrappedCache->setTTL($key, $ttl);
}

public function getTTL(string $key): int|false {
return $this->wrappedCache->getTTL($key);
}

public function compareSetTTL(string $key, mixed $value, int $ttl): bool {
Fixed Show fixed Hide fixed
return $this->wrappedCache->compareSetTTL($key, $value, $ttl);
}

public function offsetExists($offset): bool {
return $this->hasKey($offset);
}
Expand Down
15 changes: 15 additions & 0 deletions lib/private/Memcache/Redis.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ class Redis extends Cache implements IMemcacheTTL {
'if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end',
'cf0e94b2e9ffc7e04395cf88f7583fc309985910',
],
'caSetTtl' => [
'if redis.call("get", KEYS[1]) == ARGV[1] then redis.call("expire", KEYS[1], ARGV[2]) return 1 else return 0 end',
'fa4acbc946d23ef41d7d3910880b60e6e4972d72',
],
];

/**
Expand Down Expand Up @@ -181,6 +185,17 @@ public function setTTL($key, $ttl) {
$this->getCache()->expire($this->getPrefix() . $key, $ttl);
}

public function getTTL(string $key): int|false {
$ttl = $this->getCache()->ttl($this->getPrefix() . $key);
return $ttl > 0 ? (int)$ttl : false;
}

public function compareSetTTL(string $key, mixed $value, int $ttl): bool {
$value = self::encodeValue($value);

return $this->evalLua('caSetTtl', [$key], [$value, $ttl]) > 0;
}

public static function isAvailable(): bool {
return \OC::$server->getGetRedisFactory()->isAvailable();
}
Expand Down
4 changes: 3 additions & 1 deletion lib/private/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
use OCA\Theming\Util;
use OCP\Accounts\IAccountManager;
use OCP\App\IAppManager;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\Authentication\LoginCredentials\IStore;
use OCP\Authentication\Token\IProvider as OCPIProvider;
use OCP\BackgroundJob\IJobList;
Expand Down Expand Up @@ -1079,7 +1080,8 @@ public function __construct($webRoot, \OC\Config $config) {
$memcacheFactory = $c->get(ICacheFactory::class);
$memcache = $memcacheFactory->createLocking('lock');
if (!($memcache instanceof \OC\Memcache\NullCache)) {
return new MemcacheLockingProvider($memcache, $ttl);
$timeFactory = $c->get(ITimeFactory::class);
return new MemcacheLockingProvider($memcache, $timeFactory, $ttl);
}
return new DBLockingProvider(
$c->get(IDBConnection::class),
Expand Down
19 changes: 18 additions & 1 deletion lib/public/IMemcacheTTL.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,22 @@ interface IMemcacheTTL extends IMemcache {
* @param int $ttl time to live in seconds
* @since 8.2.2
*/
public function setTTL($key, $ttl);
public function setTTL(string $key, int $ttl);

/**
* Get the ttl for an existing value, in seconds till expiry
*
* @return int|false
* @since 27
*/
public function getTTL(string $key): int|false;
icewind1991 marked this conversation as resolved.
Show resolved Hide resolved
/**
* Set the ttl for an existing value if the value matches
*
* @param string $key
* @param mixed $value
* @param int $ttl time to live in seconds
* @since 27
*/
public function compareSetTTL(string $key, $value, int $ttl): bool;
}
4 changes: 3 additions & 1 deletion tests/lib/Lock/MemcacheLockingProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
namespace Test\Lock;

use OC\Memcache\ArrayCache;
use OCP\AppFramework\Utility\ITimeFactory;

class MemcacheLockingProviderTest extends LockingProvider {
/**
Expand All @@ -34,7 +35,8 @@ class MemcacheLockingProviderTest extends LockingProvider {
*/
protected function getInstance() {
$this->memcache = new ArrayCache();
return new \OC\Lock\MemcacheLockingProvider($this->memcache);
$timeProvider = \OC::$server->get(ITimeFactory::class);
return new \OC\Lock\MemcacheLockingProvider($this->memcache, $timeProvider);
}

protected function tearDown(): void {
Expand Down
21 changes: 21 additions & 0 deletions tests/lib/Memcache/RedisTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,18 @@

namespace Test\Memcache;

use OC\Memcache\Redis;

/**
* @group Memcache
* @group Redis
*/
class RedisTest extends Cache {
/**
* @var Redis cache;
*/
protected $instance;

public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();

Expand Down Expand Up @@ -62,4 +69,18 @@ public function testScriptHashes() {
$this->assertEquals(sha1($script[0]), $script[1]);
}
}

public function testCasTtlNotChanged() {
$this->instance->set('foo', 'bar', 50);
$this->assertTrue($this->instance->compareSetTTL('foo', 'bar', 100));
// allow for 1s of inaccuracy due to time moving forward
$this->assertLessThan(1, 100 - $this->instance->getTTL('foo'));
}

public function testCasTtlChanged() {
$this->instance->set('foo', 'bar1', 50);
$this->assertFalse($this->instance->compareSetTTL('foo', 'bar', 100));
// allow for 1s of inaccuracy due to time moving forward
$this->assertLessThan(1, 50 - $this->instance->getTTL('foo'));
}
}
Loading