Skip to content

Commit c0094fa

Browse files
icewind1991icewind1991
committed
fix: delay calculating global cache prefix untill a cache is created
this makes it easier to break DI circles Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent 4f84cd3 commit c0094fa

4 files changed

Lines changed: 84 additions & 48 deletions

File tree

‎lib/private/DB/QueryBuilder/Sharded/AutoIncrementHandler.php‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,22 +19,27 @@ class AutoIncrementHandler {
1919
const MIN_VALID_KEY = 1000;
2020
const TTL = 365 * 24 * 60 * 60;
2121

22-
private IMemcache $cache;
22+
private ?IMemcache $cache = null;
2323

2424
public function __construct(
25-
ICacheFactory $cacheFactory,
25+
private ICacheFactory $cacheFactory,
2626
private ShardConnectionManager $shardConnectionManager,
2727
) {
2828
if (PHP_INT_SIZE < 8) {
2929
throw new \Exception("sharding is only supported with 64bit php");
3030
}
31+
}
3132

32-
$cache = $cacheFactory->createDistributed("shared_autoincrement");
33-
if ($cache instanceof IMemcache) {
34-
$this->cache = $cache;
35-
} else {
36-
throw new \Exception('Distributed cache ' . get_class($cache) . ' is not suitable');
33+
private function getCache(): IMemcache {
34+
if(is_null($this->cache)) {
35+
$cache = $this->cacheFactory->createDistributed("shared_autoincrement");
36+
if ($cache instanceof IMemcache) {
37+
$this->cache = $cache;
38+
} else {
39+
throw new \Exception('Distributed cache ' . get_class($this->cache) . ' is not suitable');
40+
}
3741
}
42+
return $this->cache;
3843
}
3944

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

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

7075
if ($this->cache instanceof IMemcacheTTL) {
71-
$this->cache->setTTL($shardDefinition->table, self::TTL);
76+
$this->getCache()->setTTL($shardDefinition->table, self::TTL);
7277
}
7378

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

9398
// another request set the cached value before us, so we should just be able to inc
94-
$next = $this->cache->inc($shardDefinition->table);
99+
$next = $this->getCache()->inc($shardDefinition->table);
95100
if (is_int($next) && $next >= self::MIN_VALID_KEY) {
96101
return $next;
97102
} else if(is_int($next)) {
98103
// key got cleared, invalidate and retry
99-
$this->cache->cas($shardDefinition->table, $next, "empty-placeholder");
104+
$this->getCache()->cas($shardDefinition->table, $next, "empty-placeholder");
100105
return null;
101106
} else {
102107
// cleanup any non-numeric value other than the placeholder if that got stored somehow
103-
$this->cache->ncad($shardDefinition->table, "empty-placeholder");
108+
$this->getCache()->ncad($shardDefinition->table, "empty-placeholder");
104109
// retry
105110
return null;
106111
}

‎lib/private/Memcache/Factory.php‎

Lines changed: 36 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
class Factory implements ICacheFactory {
1717
public const NULL_CACHE = NullCache::class;
1818

19-
private string $globalPrefix;
19+
private ?string $globalPrefix = null;
2020

2121
private LoggerInterface $logger;
2222

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

4242
/**
43-
* @param string $globalPrefix
43+
* @param callable $globalPrefixClosure
4444
* @param LoggerInterface $logger
4545
* @param ?class-string<ICache> $localCacheClass
4646
* @param ?class-string<ICache> $distributedCacheClass
4747
* @param ?class-string<IMemcache> $lockingCacheClass
4848
* @param string $logFile
4949
*/
50-
public function __construct(string $globalPrefix, LoggerInterface $logger, IProfiler $profiler,
51-
?string $localCacheClass = null, ?string $distributedCacheClass = null, ?string $lockingCacheClass = null, string $logFile = '') {
50+
public function __construct(
51+
private $globalPrefixClosure,
52+
LoggerInterface $logger,
53+
IProfiler $profiler,
54+
?string $localCacheClass = null,
55+
?string $distributedCacheClass = null,
56+
?string $lockingCacheClass = null,
57+
string $logFile = ''
58+
) {
5259
$this->logFile = $logFile;
53-
$this->globalPrefix = $globalPrefix;
5460

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

94+
private function getGlobalPrefix(): ?string {
95+
if (is_null($this->globalPrefix)) {
96+
$this->globalPrefix = ($this->globalPrefixClosure)();
97+
}
98+
return $this->globalPrefix;
99+
}
100+
88101
/**
89102
* create a cache instance for storing locks
90103
*
91104
* @param string $prefix
92105
* @return IMemcache
93106
*/
94107
public function createLocking(string $prefix = ''): IMemcache {
108+
$globalPrefix = $this->getGlobalPrefix();
109+
if (is_null($globalPrefix)) {
110+
return new ArrayCache($prefix);
111+
}
112+
95113
assert($this->lockingCacheClass !== null);
96-
$cache = new $this->lockingCacheClass($this->globalPrefix . '/' . $prefix);
114+
$cache = new $this->lockingCacheClass($globalPrefix . '/' . $prefix);
97115
if ($this->lockingCacheClass === Redis::class && $this->profiler->isEnabled()) {
98116
// We only support the profiler with Redis
99117
$cache = new ProfilerWrapperCache($cache, 'Locking');
@@ -114,8 +132,13 @@ public function createLocking(string $prefix = ''): IMemcache {
114132
* @return ICache
115133
*/
116134
public function createDistributed(string $prefix = ''): ICache {
135+
$globalPrefix = $this->getGlobalPrefix();
136+
if (is_null($globalPrefix)) {
137+
return new ArrayCache($prefix);
138+
}
139+
117140
assert($this->distributedCacheClass !== null);
118-
$cache = new $this->distributedCacheClass($this->globalPrefix . '/' . $prefix);
141+
$cache = new $this->distributedCacheClass($globalPrefix . '/' . $prefix);
119142
if ($this->distributedCacheClass === Redis::class && $this->profiler->isEnabled()) {
120143
// We only support the profiler with Redis
121144
$cache = new ProfilerWrapperCache($cache, 'Distributed');
@@ -136,8 +159,13 @@ public function createDistributed(string $prefix = ''): ICache {
136159
* @return ICache
137160
*/
138161
public function createLocal(string $prefix = ''): ICache {
162+
$globalPrefix = $this->getGlobalPrefix();
163+
if (is_null($globalPrefix)) {
164+
return new ArrayCache($prefix);
165+
}
166+
139167
assert($this->localCacheClass !== null);
140-
$cache = new $this->localCacheClass($this->globalPrefix . '/' . $prefix);
168+
$cache = new $this->localCacheClass($globalPrefix . '/' . $prefix);
141169
if ($this->localCacheClass === Redis::class && $this->profiler->isEnabled()) {
142170
// We only support the profiler with Redis
143171
$cache = new ProfilerWrapperCache($cache, 'Local');

‎lib/private/Server.php‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,7 @@ public function __construct($webRoot, \OC\Config $config) {
637637

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

649649
if ($config->getValue('installed', false) && !(defined('PHPUNIT_RUN') && PHPUNIT_RUN)) {
650-
if (!$config->getValue('log_query')) {
651-
try {
652-
$v = \OC_App::getAppVersions();
653-
} catch (\Doctrine\DBAL\Exception $e) {
654-
// Database service probably unavailable
655-
// Probably related to https://github.com/nextcloud/server/issues/37424
656-
return $arrayCacheFactory;
650+
$logQuery = $config->getValue('log_query');
651+
$prefixClosure = function() use ($logQuery) {
652+
if (!$logQuery) {
653+
try {
654+
$v = \OC_App::getAppVersions();
655+
} catch (\Doctrine\DBAL\Exception $e) {
656+
// Database service probably unavailable
657+
// Probably related to https://github.com/nextcloud/server/issues/37424
658+
return null;
659+
}
660+
} else {
661+
// If the log_query is enabled, we can not get the app versions
662+
// as that does a query, which will be logged and the logging
663+
// depends on redis and here we are back again in the same function.
664+
$v = [
665+
'log_query' => 'enabled',
666+
];
657667
}
658-
} else {
659-
// If the log_query is enabled, we can not get the app versions
660-
// as that does a query, which will be logged and the logging
661-
// depends on redis and here we are back again in the same function.
662-
$v = [
663-
'log_query' => 'enabled',
664-
];
665-
}
666-
$v['core'] = implode(',', \OC_Util::getVersion());
667-
$version = implode(',', $v);
668-
$instanceId = \OC_Util::getInstanceId();
669-
$path = \OC::$SERVERROOT;
670-
$prefix = md5($instanceId . '-' . $version . '-' . $path);
671-
return new \OC\Memcache\Factory($prefix,
668+
$v['core'] = implode(',', \OC_Util::getVersion());
669+
$version = implode(',', $v);
670+
$instanceId = \OC_Util::getInstanceId();
671+
$path = \OC::$SERVERROOT;
672+
return md5($instanceId . '-' . $version . '-' . $path);
673+
};
674+
return new \OC\Memcache\Factory($prefixClosure,
672675
$c->get(LoggerInterface::class),
673676
$profiler,
674677
$config->getValue('memcache.local', null),

‎tests/lib/Memcache/FactoryTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ public function testCacheAvailability($localCache, $distributedCache, $lockingCa
110110
$expectedLocalCache, $expectedDistributedCache, $expectedLockingCache) {
111111
$logger = $this->getMockBuilder(LoggerInterface::class)->getMock();
112112
$profiler = $this->getMockBuilder(IProfiler::class)->getMock();
113-
$factory = new \OC\Memcache\Factory('abc', $logger, $profiler, $localCache, $distributedCache, $lockingCache);
113+
$factory = new \OC\Memcache\Factory(fn() => 'abc', $logger, $profiler, $localCache, $distributedCache, $lockingCache);
114114
$this->assertTrue(is_a($factory->createLocal(), $expectedLocalCache));
115115
$this->assertTrue(is_a($factory->createDistributed(), $expectedDistributedCache));
116116
$this->assertTrue(is_a($factory->createLocking(), $expectedLockingCache));
@@ -124,13 +124,13 @@ public function testCacheNotAvailableException($localCache, $distributedCache) {
124124

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

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

135135
$cache = $factory->createInMemory();
136136
$cache->set('test', 48);

0 commit comments

Comments
 (0)