Skip to content

Commit

Permalink
fix: delay calculating global cache prefix untill a cache is created
Browse files Browse the repository at this point in the history 
this makes it easier to break DI circles

Signed-off-by: Robin Appelman <robin@icewind.nl>
  • Loading branch information
@icewind1991
icewind1991 committed Aug 9, 2024
1 parent 700e734 commit 80aad73
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 48 deletions.
35 changes: 20 additions & 15 deletions lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,27 @@ class AutoIncrementHandler {
const MIN_VALID_KEY = 1000;
const TTL = 365 * 24 * 60 * 60;

private IMemcache $cache;
private ?IMemcache $cache = null;

public function __construct(
ICacheFactory $cacheFactory,
private ICacheFactory $cacheFactory,
private ShardConnectionManager $shardConnectionManager,
) {
if (PHP_INT_SIZE < 8) {
throw new \Exception("sharding is only supported with 64bit php");
}
}

$cache = $cacheFactory->createDistributed("shared_autoincrement");
if ($cache instanceof IMemcache) {
$this->cache = $cache;
} else {
throw new \Exception('Distributed cache ' . get_class($cache) . ' is not suitable');
private function getCache(): IMemcache {
if(is_null($this->cache)) {
$cache = $this->cacheFactory->createDistributed("shared_autoincrement");
if ($cache instanceof IMemcache) {
$this->cache = $cache;
} else {
throw new \Exception('Distributed cache ' . get_class($this->cache) . ' is not suitable');

Check failure

Code scanning / Psalm

NullArgument Error

Argument 1 of get_class cannot be null, null value provided to parameter with type object

Check failure on line 39 in lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

NullArgument 

lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php:39:59: NullArgument: Argument 1 of get_class cannot be null, null value provided to parameter with type object (see https://psalm.dev/057)
}
}
return $this->cache;
}

public function getNextPrimaryKey(ShardDefinition $shardDefinition): int {
Expand Down Expand Up @@ -64,11 +69,11 @@ private function getNextPrimaryKeyInner(ShardDefinition $shardDefinition): ?int
// care must be taken that the logic remains fully resilient against race conditions

// prevent inc from returning `1` if the key doesn't exist by setting it to a non-numeric value
$this->cache->add($shardDefinition->table, "empty-placeholder", self::TTL);
$next = $this->cache->inc($shardDefinition->table);
$this->getCache()->add($shardDefinition->table, "empty-placeholder", self::TTL);
$next = $this->getCache()->inc($shardDefinition->table);

if ($this->cache instanceof IMemcacheTTL) {
$this->cache->setTTL($shardDefinition->table, self::TTL);
$this->getCache()->setTTL($shardDefinition->table, self::TTL);

Check failure

Code scanning / Psalm

UndefinedInterfaceMethod Error

Method OCP\IMemcache::setTTL does not exist

Check failure on line 76 in lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php

View workflow job for this annotation

GitHub Actions / static-code-analysis

UndefinedInterfaceMethod 

lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php:76:23: UndefinedInterfaceMethod: Method OCP\IMemcache::setTTL does not exist (see https://psalm.dev/181)
}

// the "add + inc" trick above isn't strictly atomic, so as a safety we reject any result that to small
Expand All @@ -77,7 +82,7 @@ private function getNextPrimaryKeyInner(ShardDefinition $shardDefinition): ?int
return $next;
} elseif (is_int($next)) {
// we hit the edge case, so invalidate the cached value
if (!$this->cache->cas($shardDefinition->table, $next, "empty-placeholder")) {
if (!$this->getCache()->cas($shardDefinition->table, $next, "empty-placeholder")) {
// someone else is changing the value concurrently, give up and retry
return null;
}
Expand All @@ -86,21 +91,21 @@ private function getNextPrimaryKeyInner(ShardDefinition $shardDefinition): ?int
// discard the encoded initial shard
$current = $this->getMaxFromDb($shardDefinition) >> 8;
$next = max($current, self::MIN_VALID_KEY) + 1;
if ($this->cache->cas($shardDefinition->table, "empty-placeholder", $next)) {
if ($this->getCache()->cas($shardDefinition->table, "empty-placeholder", $next)) {
return $next;
}

// another request set the cached value before us, so we should just be able to inc
$next = $this->cache->inc($shardDefinition->table);
$next = $this->getCache()->inc($shardDefinition->table);
if (is_int($next) && $next >= self::MIN_VALID_KEY) {
return $next;
} else if(is_int($next)) {
// key got cleared, invalidate and retry
$this->cache->cas($shardDefinition->table, $next, "empty-placeholder");
$this->getCache()->cas($shardDefinition->table, $next, "empty-placeholder");
return null;
} else {
// cleanup any non-numeric value other than the placeholder if that got stored somehow
$this->cache->ncad($shardDefinition->table, "empty-placeholder");
$this->getCache()->ncad($shardDefinition->table, "empty-placeholder");
// retry
return null;
}
Expand Down
44 changes: 36 additions & 8 deletions lib/private/Memcache/Factory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
class Factory implements ICacheFactory {
public const NULL_CACHE = NullCache::class;

private string $globalPrefix;
private ?string $globalPrefix = null;

private LoggerInterface $logger;

Expand All @@ -40,17 +40,23 @@ class Factory implements ICacheFactory {
private IProfiler $profiler;

/**
* @param string $globalPrefix
* @param callable $globalPrefixClosure
* @param LoggerInterface $logger
* @param ?class-string<ICache> $localCacheClass
* @param ?class-string<ICache> $distributedCacheClass
* @param ?class-string<IMemcache> $lockingCacheClass
* @param string $logFile
*/
public function __construct(string $globalPrefix, LoggerInterface $logger, IProfiler $profiler,
?string $localCacheClass = null, ?string $distributedCacheClass = null, ?string $lockingCacheClass = null, string $logFile = '') {
public function __construct(
private $globalPrefixClosure,
LoggerInterface $logger,
IProfiler $profiler,
?string $localCacheClass = null,
?string $distributedCacheClass = null,
?string $lockingCacheClass = null,
string $logFile = ''
) {
$this->logFile = $logFile;
$this->globalPrefix = $globalPrefix;

if (!$localCacheClass) {
$localCacheClass = self::NULL_CACHE;
Expand Down Expand Up @@ -85,15 +91,27 @@ public function __construct(string $globalPrefix, LoggerInterface $logger, IProf
$this->profiler = $profiler;
}

private function getGlobalPrefix(): ?string {
if (is_null($this->globalPrefix)) {
$this->globalPrefix = ($this->globalPrefixClosure)();
}
return $this->globalPrefix;
}

/**
* create a cache instance for storing locks
*
* @param string $prefix
* @return IMemcache
*/
public function createLocking(string $prefix = ''): IMemcache {
$globalPrefix = $this->getGlobalPrefix();
if (is_null($globalPrefix)) {
return new ArrayCache($prefix);
}

assert($this->lockingCacheClass !== null);
$cache = new $this->lockingCacheClass($this->globalPrefix . '/' . $prefix);
$cache = new $this->lockingCacheClass($globalPrefix . '/' . $prefix);

Check failure

Code scanning / Psalm

TaintedCallable Error

Detected tainted text
if ($this->lockingCacheClass === Redis::class && $this->profiler->isEnabled()) {
// We only support the profiler with Redis
$cache = new ProfilerWrapperCache($cache, 'Locking');
Expand All @@ -114,8 +132,13 @@ public function createLocking(string $prefix = ''): IMemcache {
* @return ICache
*/
public function createDistributed(string $prefix = ''): ICache {
$globalPrefix = $this->getGlobalPrefix();
if (is_null($globalPrefix)) {
return new ArrayCache($prefix);
}

assert($this->distributedCacheClass !== null);
$cache = new $this->distributedCacheClass($this->globalPrefix . '/' . $prefix);
$cache = new $this->distributedCacheClass($globalPrefix . '/' . $prefix);

Check failure

Code scanning / Psalm

TaintedCallable Error

Detected tainted text
if ($this->distributedCacheClass === Redis::class && $this->profiler->isEnabled()) {
// We only support the profiler with Redis
$cache = new ProfilerWrapperCache($cache, 'Distributed');
Expand All @@ -136,8 +159,13 @@ public function createDistributed(string $prefix = ''): ICache {
* @return ICache
*/
public function createLocal(string $prefix = ''): ICache {
$globalPrefix = $this->getGlobalPrefix();
if (is_null($globalPrefix)) {
return new ArrayCache($prefix);
}

assert($this->localCacheClass !== null);
$cache = new $this->localCacheClass($this->globalPrefix . '/' . $prefix);
$cache = new $this->localCacheClass($globalPrefix . '/' . $prefix);

Check failure

Code scanning / Psalm

TaintedCallable Error

Detected tainted text
if ($this->localCacheClass === Redis::class && $this->profiler->isEnabled()) {
// We only support the profiler with Redis
$cache = new ProfilerWrapperCache($cache, 'Local');
Expand Down
47 changes: 25 additions & 22 deletions lib/private/Server.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ public function __construct($webRoot, \OC\Config $config) {

$this->registerService(Factory::class, function (Server $c) {
$profiler = $c->get(IProfiler::class);
$arrayCacheFactory = new \OC\Memcache\Factory('', $c->get(LoggerInterface::class),
$arrayCacheFactory = new \OC\Memcache\Factory(fn () => '', $c->get(LoggerInterface::class),
$profiler,
ArrayCache::class,
ArrayCache::class,
Expand All @@ -647,28 +647,31 @@ public function __construct($webRoot, \OC\Config $config) {
$config = $c->get(SystemConfig::class);

if ($config->getValue('installed', false) && !(defined('PHPUNIT_RUN') && PHPUNIT_RUN)) {
if (!$config->getValue('log_query')) {
try {
$v = \OC_App::getAppVersions();
} catch (\Doctrine\DBAL\Exception $e) {
// Database service probably unavailable
// Probably related to https://github.com/nextcloud/server/issues/37424
return $arrayCacheFactory;
$logQuery = $config->getValue('log_query');
$prefixClosure = function() use ($logQuery) {
if (!$logQuery) {
try {
$v = \OC_App::getAppVersions();
} catch (\Doctrine\DBAL\Exception $e) {
// Database service probably unavailable
// Probably related to https://github.com/nextcloud/server/issues/37424
return null;
}
} else {
// If the log_query is enabled, we can not get the app versions
// as that does a query, which will be logged and the logging
// depends on redis and here we are back again in the same function.
$v = [
'log_query' => 'enabled',
];
}
} else {
// If the log_query is enabled, we can not get the app versions
// as that does a query, which will be logged and the logging
// depends on redis and here we are back again in the same function.
$v = [
'log_query' => 'enabled',
];
}
$v['core'] = implode(',', \OC_Util::getVersion());
$version = implode(',', $v);
$instanceId = \OC_Util::getInstanceId();
$path = \OC::$SERVERROOT;
$prefix = md5($instanceId . '-' . $version . '-' . $path);
return new \OC\Memcache\Factory($prefix,
$v['core'] = implode(',', \OC_Util::getVersion());
$version = implode(',', $v);
$instanceId = \OC_Util::getInstanceId();
$path = \OC::$SERVERROOT;
return md5($instanceId . '-' . $version . '-' . $path);
};
return new \OC\Memcache\Factory($prefixClosure,
$c->get(LoggerInterface::class),
$profiler,
$config->getValue('memcache.local', null),
Expand Down
6 changes: 3 additions & 3 deletions tests/lib/Memcache/FactoryTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public function testCacheAvailability($localCache, $distributedCache, $lockingCa
$expectedLocalCache, $expectedDistributedCache, $expectedLockingCache) {
$logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
$profiler = $this->getMockBuilder(IProfiler::class)->getMock();
$factory = new \OC\Memcache\Factory('abc', $logger, $profiler, $localCache, $distributedCache, $lockingCache);
$factory = new \OC\Memcache\Factory(fn() => 'abc', $logger, $profiler, $localCache, $distributedCache, $lockingCache);
$this->assertTrue(is_a($factory->createLocal(), $expectedLocalCache));
$this->assertTrue(is_a($factory->createDistributed(), $expectedDistributedCache));
$this->assertTrue(is_a($factory->createLocking(), $expectedLockingCache));
Expand All @@ -124,13 +124,13 @@ public function testCacheNotAvailableException($localCache, $distributedCache) {

$logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
$profiler = $this->getMockBuilder(IProfiler::class)->getMock();
new \OC\Memcache\Factory('abc', $logger, $profiler, $localCache, $distributedCache);
new \OC\Memcache\Factory(fn() => 'abc', $logger, $profiler, $localCache, $distributedCache);
}

public function testCreateInMemory(): void {
$logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
$profiler = $this->getMockBuilder(IProfiler::class)->getMock();
$factory = new \OC\Memcache\Factory('abc', $logger, $profiler, null, null, null);
$factory = new \OC\Memcache\Factory(fn() => 'abc', $logger, $profiler, null, null, null);

$cache = $factory->createInMemory();
$cache->set('test', 48);
Expand Down

0 comments on commit 80aad73

Please sign in to comment.