Skip to content

Commit 06662da

Browse files
solracsfsolracsf
committed
fix(filecache): announce every removed entry so metadata is cleaned up
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent 18dbb9c commit 06662da

2 files changed

Lines changed: 200 additions & 7 deletions

File tree

‎lib/private/Files/Cache/Cache.php‎

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,9 @@ public function remove($file) {
607607
$this->removeChildren($entry);
608608
}
609609

610-
$this->eventDispatcher->dispatchTyped(new CacheEntryRemovedEvent($this->storage, $entry->getPath(), $entry->getId(), $this->getNumericStorageId()));
610+
$event = new CacheEntryRemovedEvent($this->storage, $entry->getPath(), $entry->getId(), $this->getNumericStorageId());
611+
$this->eventDispatcher->dispatchTyped($event);
612+
$this->eventDispatcher->dispatchTyped(new CacheEntriesRemovedEvent([$event]));
611613
}
612614
}
613615

@@ -678,8 +680,8 @@ private function removeChildren(ICacheEntry $entry) {
678680
$query->executeStatement();
679681
}
680682

681-
$cacheEntryRemovedEvents = [];
682-
foreach (array_chunk(array_combine($deletedIds, $deletedPaths), IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) {
683+
foreach (array_chunk(array_combine($deletedIds, $deletedPaths), IQueryBuilder::MAX_IN_PARAMETERS, true) as $chunk) {
684+
$cacheEntryRemovedEvents = [];
683685
/** @var array<int, string> $chunk */
684686
foreach ($chunk as $fileId => $filePath) {
685687
$cacheEntryRemovedEvents[] = new CacheEntryRemovedEvent(
@@ -899,15 +901,53 @@ private function getChildIds(int $storageId, string $path): array {
899901
* remove all entries for files that are stored on the storage from the cache
900902
*/
901903
public function clear() {
902-
$query = $this->getQueryBuilder();
903-
$query->delete('filecache')
904-
->whereStorageId($this->getNumericStorageId());
905-
$query->executeStatement();
904+
$storageId = $this->getNumericStorageId();
905+
$exception = null;
906+
907+
// removed in batches so a storage with many entries does not have to be held in memory at once
908+
while (true) {
909+
$query = $this->getQueryBuilder();
910+
$query->select('fileid', 'path')
911+
->from('filecache')
912+
->whereStorageId($storageId)
913+
->setMaxResults(IQueryBuilder::MAX_IN_PARAMETERS);
914+
$rows = $query->executeQuery()->fetchAll();
915+
if ($rows === []) {
916+
break;
917+
}
918+
919+
$fileIds = array_map(static fn (array $row): int => (int)$row['fileid'], $rows);
920+
921+
$query = $this->getQueryBuilder();
922+
$query->delete('filecache')
923+
->whereStorageId($storageId)
924+
->andWhere($query->expr()->in('fileid', $query->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)));
925+
$query->executeStatement();
926+
927+
$cacheEntryRemovedEvents = [];
928+
foreach ($rows as $row) {
929+
$cacheEntryRemovedEvents[] = new CacheEntryRemovedEvent($this->storage, (string)$row['path'], (int)$row['fileid'], $storageId);
930+
}
931+
932+
// a listener must not be able to leave the storage half cleared
933+
try {
934+
$this->eventDispatcher->dispatchTyped(new CacheEntriesRemovedEvent($cacheEntryRemovedEvents));
935+
foreach ($cacheEntryRemovedEvents as $cacheEntryRemovedEvent) {
936+
$this->eventDispatcher->dispatchTyped($cacheEntryRemovedEvent);
937+
}
938+
} catch (\Exception $e) {
939+
$exception ??= $e;
940+
}
941+
}
906942

907943
$query = $this->connection->getQueryBuilder();
908944
$query->delete('storages')
909945
->where($query->expr()->eq('id', $query->createNamedParameter($this->storageId)));
910946
$query->executeStatement();
947+
948+
if ($exception !== null) {
949+
throw $exception;
950+
}
911951
}
912952

913953
/**

‎tests/lib/Files/Cache/CacheTest.php‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,30 @@
99
namespace Test\Files\Cache;
1010

1111
use OC\Files\Cache\Cache;
12+
use OC\Files\Cache\CacheDependencies;
1213
use OC\Files\Cache\CacheEntry;
14+
use OC\Files\Cache\QuerySearchHelper;
1315
use OC\Files\Cache\Wrapper\CacheJail;
1416
use OC\Files\Search\SearchComparison;
1517
use OC\Files\Search\SearchQuery;
1618
use OC\Files\Storage\Temporary;
19+
use OC\SystemConfig;
20+
use OC\User\DisplayNameCache;
1721
use OC\User\User;
22+
use OCP\DB\QueryBuilder\IQueryBuilder;
1823
use OCP\EventDispatcher\IEventDispatcher;
24+
use OCP\Files\Cache\CacheEntriesRemovedEvent;
1925
use OCP\Files\Cache\ICacheEntry;
26+
use OCP\Files\IMimeTypeLoader;
2027
use OCP\Files\Search\ISearchComparison;
28+
use OCP\Files\Storage\IStorage;
29+
use OCP\FilesMetadata\IFilesMetadataManager;
2130
use OCP\IDBConnection;
2231
use OCP\ITagManager;
2332
use OCP\IUser;
2433
use OCP\IUserManager;
2534
use OCP\Server;
35+
use Psr\Log\LoggerInterface;
2636

2737
class LongId extends Temporary {
2838
#[\Override]
@@ -245,6 +255,149 @@ public function testRemoveRecursive(): void {
245255
}
246256
}
247257

258+
/**
259+
* @return array{0: Cache, 1: \Closure(): list<CacheEntriesRemovedEvent>}
260+
*/
261+
private function cacheWithRecordedEvents(IStorage $storage): array {
262+
$events = [];
263+
$dispatcher = $this->createMock(IEventDispatcher::class);
264+
$dispatcher->method('dispatchTyped')
265+
->willReturnCallback(function (object $event) use (&$events): void {
266+
if ($event instanceof CacheEntriesRemovedEvent) {
267+
$events[] = $event;
268+
}
269+
});
270+
271+
$dependencies = new CacheDependencies(
272+
Server::get(IMimeTypeLoader::class),
273+
Server::get(IDBConnection::class),
274+
$dispatcher,
275+
Server::get(QuerySearchHelper::class),
276+
Server::get(SystemConfig::class),
277+
Server::get(LoggerInterface::class),
278+
Server::get(IFilesMetadataManager::class),
279+
Server::get(DisplayNameCache::class),
280+
);
281+
282+
return [new Cache($storage, $dependencies), function () use (&$events): array {
283+
return $events;
284+
}];
285+
}
286+
287+
/**
288+
* @param list<CacheEntriesRemovedEvent> $events
289+
* @return list<int>
290+
*/
291+
private function announcedFileIds(array $events): array {
292+
$fileIds = [];
293+
foreach ($events as $event) {
294+
foreach ($event->getCacheEntryRemovedEvents() as $removed) {
295+
$fileIds[] = $removed->getFileId();
296+
}
297+
}
298+
return $fileIds;
299+
}
300+
301+
public function testRemoveSingleFileAnnouncesBatchEvent(): void {
302+
$storage = new Temporary([]);
303+
[$cache, $recorded] = $this->cacheWithRecordedEvents($storage);
304+
$cache->insert('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
305+
$fileId = $cache->put('foo.txt', ['size' => 1, 'mtime' => 20, 'mimetype' => 'text/plain']);
306+
307+
$cache->remove('foo.txt');
308+
309+
$this->assertEquals([$fileId], $this->announcedFileIds($recorded()));
310+
}
311+
312+
public function testRemoveRecursiveAnnouncesRealFileIds(): void {
313+
$storage = new Temporary([]);
314+
[$cache, $recorded] = $this->cacheWithRecordedEvents($storage);
315+
$cache->insert('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
316+
$folderData = ['size' => 100, 'mtime' => 50, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE];
317+
$fileData = ['size' => 1000, 'mtime' => 20, 'mimetype' => 'text/plain'];
318+
319+
$expected = [$cache->put('folder', $folderData)];
320+
$expected[] = $cache->put('folder/sub', $folderData);
321+
$expected[] = $cache->put('folder/foo.txt', $fileData);
322+
$expected[] = $cache->put('folder/sub/bar.txt', $fileData);
323+
324+
$cache->remove('folder');
325+
326+
$announced = $this->announcedFileIds($recorded());
327+
sort($expected);
328+
sort($announced);
329+
$this->assertEquals($expected, $announced);
330+
}
331+
332+
public function testRemoveRecursiveAnnouncesEveryChildExactlyOnce(): void {
333+
$storage = new Temporary([]);
334+
[$cache, $recorded] = $this->cacheWithRecordedEvents($storage);
335+
$cache->insert('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
336+
$fileData = ['size' => 1, 'mtime' => 20, 'mimetype' => 'text/plain'];
337+
338+
$expected = [$cache->put('folder', ['size' => 0, 'mtime' => 50, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE])];
339+
for ($i = 0; $i < IQueryBuilder::MAX_IN_PARAMETERS + 1; $i++) {
340+
$expected[] = $cache->insert("folder/child$i.txt", $fileData);
341+
}
342+
343+
$cache->remove('folder');
344+
345+
$announced = $this->announcedFileIds($recorded());
346+
$this->assertCount(count($expected), $announced, 'every removed entry is announced exactly once');
347+
sort($expected);
348+
sort($announced);
349+
$this->assertEquals($expected, $announced);
350+
}
351+
352+
public function testClearEmptiesTheStorageEvenIfAListenerThrows(): void {
353+
$storage = new Temporary([]);
354+
$dispatcher = $this->createMock(IEventDispatcher::class);
355+
$dispatcher->method('dispatchTyped')
356+
->willReturnCallback(function (object $event): void {
357+
if ($event instanceof CacheEntriesRemovedEvent) {
358+
throw new \RuntimeException('listener blew up');
359+
}
360+
});
361+
$dependencies = new CacheDependencies(
362+
Server::get(IMimeTypeLoader::class),
363+
Server::get(IDBConnection::class),
364+
$dispatcher,
365+
Server::get(QuerySearchHelper::class),
366+
Server::get(SystemConfig::class),
367+
Server::get(LoggerInterface::class),
368+
Server::get(IFilesMetadataManager::class),
369+
Server::get(DisplayNameCache::class),
370+
);
371+
$cache = new Cache($storage, $dependencies);
372+
$cache->insert('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE]);
373+
$cache->put('foo.txt', ['size' => 1, 'mtime' => 20, 'mimetype' => 'text/plain']);
374+
375+
try {
376+
$cache->clear();
377+
$this->fail('the listener exception should surface');
378+
} catch (\RuntimeException $e) {
379+
$this->assertEquals('listener blew up', $e->getMessage());
380+
}
381+
382+
$this->assertFalse($cache->inCache('foo.txt'));
383+
$this->assertFalse($cache->inCache(''));
384+
}
385+
386+
public function testClearAnnouncesRemovedEntries(): void {
387+
$storage = new Temporary([]);
388+
[$cache, $recorded] = $this->cacheWithRecordedEvents($storage);
389+
$expected = [$cache->insert('', ['size' => 0, 'mtime' => 0, 'mimetype' => ICacheEntry::DIRECTORY_MIMETYPE])];
390+
$expected[] = $cache->put('foo.txt', ['size' => 1, 'mtime' => 20, 'mimetype' => 'text/plain']);
391+
$expected[] = $cache->put('bar.txt', ['size' => 1, 'mtime' => 20, 'mimetype' => 'text/plain']);
392+
393+
$cache->clear();
394+
395+
$announced = $this->announcedFileIds($recorded());
396+
sort($expected);
397+
sort($announced);
398+
$this->assertEquals($expected, $announced);
399+
}
400+
248401
public static function folderDataProvider(): array {
249402
return [
250403
['folder'],

0 commit comments

Comments
 (0)