Skip to content

Commit 0b369eb

Browse files
artongeartonge
committed
fix(Scanner): Remove high level transaction during scans
Signed-off-by: Louis Chmn <louis@chmn.me>
1 parent 30f495a commit 0b369eb

7 files changed

Lines changed: 85 additions & 53 deletions

File tree

‎apps/files/lib/BackgroundJob/ScanFiles.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,15 @@
88

99
namespace OCA\Files\BackgroundJob;
1010

11+
use OC\Files\SetupManager;
1112
use OC\Files\Utils\Scanner;
1213
use OCP\AppFramework\Utility\ITimeFactory;
1314
use OCP\BackgroundJob\TimedJob;
1415
use OCP\DB\QueryBuilder\IQueryBuilder;
1516
use OCP\EventDispatcher\IEventDispatcher;
1617
use OCP\IConfig;
1718
use OCP\IDBConnection;
19+
use OCP\IUserManager;
1820
use Psr\Log\LoggerInterface;
1921

2022
/**
@@ -33,6 +35,8 @@ public function __construct(
3335
private LoggerInterface $logger,
3436
private IDBConnection $connection,
3537
ITimeFactory $time,
38+
private readonly SetupManager $setupManager,
39+
private readonly IUserManager $userManager,
3640
) {
3741
parent::__construct($time);
3842
// Run once per 10 minutes
@@ -45,7 +49,9 @@ protected function runScanner(string $user): void {
4549
$user,
4650
null,
4751
$this->dispatcher,
48-
$this->logger
52+
$this->logger,
53+
$this->setupManager,
54+
$this->userManager,
4955
);
5056
$scanner->backgroundScan('');
5157
} catch (\Exception $e) {

‎apps/files/lib/Command/Scan.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OC\Core\Command\InterruptedException;
1212
use OC\DB\Connection;
1313
use OC\DB\ConnectionAdapter;
14+
use OC\Files\SetupManager;
1415
use OC\Files\Storage\Wrapper\Jail;
1516
use OC\Files\Utils\Scanner;
1617
use OC\FilesMetadata\FilesMetadataManager;
@@ -114,7 +115,9 @@ protected function scanFiles(
114115
$user,
115116
new ConnectionAdapter($connection),
116117
Server::get(IEventDispatcher::class),
117-
Server::get(LoggerInterface::class)
118+
Server::get(LoggerInterface::class),
119+
Server::get(SetupManager::class),
120+
Server::get(IUserManager::class),
118121
);
119122

120123
# check on each file/folder if there was a user interrupt (ctrl-c) and throw an exception

‎apps/files/lib/Command/ScanAppData.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use OC\Core\Command\InterruptedException;
1111
use OC\DB\Connection;
1212
use OC\DB\ConnectionAdapter;
13+
use OC\Files\SetupManager;
1314
use OC\Files\Utils\Scanner;
1415
use OC\ForbiddenException;
1516
use OC\Preview\Storage\StorageFactory;
@@ -20,6 +21,7 @@
2021
use OCP\Files\NotFoundException;
2122
use OCP\Files\StorageNotAvailableException;
2223
use OCP\IConfig;
24+
use OCP\IUserManager;
2325
use OCP\Server;
2426
use Psr\Log\LoggerInterface;
2527
use Symfony\Component\Console\Helper\Table;
@@ -60,6 +62,8 @@ protected function getScanner(OutputInterface $output): Scanner {
6062
new ConnectionAdapter($connection),
6163
Server::get(IEventDispatcher::class),
6264
Server::get(LoggerInterface::class),
65+
Server::get(SetupManager::class),
66+
Server::get(IUserManager::class),
6367
);
6468
}
6569

‎apps/files/tests/BackgroundJob/ScanFilesTest.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
namespace OCA\Files\Tests\BackgroundJob;
1010

1111
use OC\Files\Mount\MountPoint;
12+
use OC\Files\SetupManager;
1213
use OC\Files\Storage\Temporary;
1314
use OCA\Files\BackgroundJob\ScanFiles;
1415
use OCP\AppFramework\Utility\ITimeFactory;
@@ -17,6 +18,7 @@
1718
use OCP\IConfig;
1819
use OCP\IDBConnection;
1920
use OCP\IUser;
21+
use OCP\IUserManager;
2022
use OCP\Server;
2123
use Psr\Log\LoggerInterface;
2224
use Test\TestCase;
@@ -51,7 +53,9 @@ protected function setUp(): void {
5153
$dispatcher,
5254
$logger,
5355
$connection,
54-
$this->createMock(ITimeFactory::class)
56+
$this->createMock(ITimeFactory::class),
57+
$this->createMock(SetupManager::class),
58+
$this->createMock(IUserManager::class),
5559
])
5660
->onlyMethods(['runScanner'])
5761
->getMock();

‎lib/private/Files/Utils/Scanner.php‎

Lines changed: 7 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use OC\Hooks\PublicEmitter;
1818
use OC\Lock\DBLockingProvider;
1919
use OCA\Files_Sharing\SharedStorage;
20+
use OCP\AppFramework\Utility\ITimeFactory;
2021
use OCP\EventDispatcher\IEventDispatcher;
2122
use OCP\Files\Events\BeforeFileScannedEvent;
2223
use OCP\Files\Events\BeforeFolderScannedEvent;
@@ -46,17 +47,6 @@
4647
* @package OC\Files\Utils
4748
*/
4849
class Scanner extends PublicEmitter {
49-
public const MAX_ENTRIES_TO_COMMIT = 10000;
50-
51-
/**
52-
* Whether to use a DB transaction
53-
*/
54-
protected bool $useTransaction;
55-
56-
/**
57-
* Number of entries scanned to commit
58-
*/
59-
protected int $entriesToCommit = 0;
6050

6151
public function __construct(
6252
private readonly ?string $user,
@@ -66,8 +56,6 @@ public function __construct(
6656
private readonly SetupManager $setupManager,
6757
private readonly IUserManager $userManager,
6858
) {
69-
// when DB locking is used, no DB transactions will be used
70-
$this->useTransaction = !(Server::get(ILockingProvider::class) instanceof DBLockingProvider);
7159
}
7260

7361
/**
@@ -80,13 +68,11 @@ protected function getMounts($dir) {
8068
//TODO: move to the node based fileapi once that's done
8169
$this->setupManager->tearDown();
8270

83-
$userObject = $this->userManager->get($this->user);
84-
if ($userObject === null) {
85-
throw new \InvalidArgumentException("User {$this->user} does not exist");
71+
if ($this->user !== null) {
72+
$userObject = $this->userManager->get($this->user);
73+
$this->setupManager->setupForUser($userObject);
8674
}
8775

88-
$this->setupManager->setupForUser($userObject);
89-
9076
$mountManager = Filesystem::getMountManager();
9177
$mounts = $mountManager->findIn($dir);
9278
$mounts[] = $mountManager->find($dir);
@@ -216,19 +202,18 @@ public function scan(string $dir = '', $recursive = \OC\Files\Cache\Scanner::SCA
216202
$relativePath = $mount->getInternalPath($dir);
217203
/** @var \OC\Files\Cache\Scanner $scanner */
218204
$scanner = $storage->getScanner();
219-
$scanner->setUseTransactions(false);
220205
$this->attachListener($mount);
221206

222207
$scanner->listen('\OC\Files\Cache\Scanner', 'removeFromCache', function ($path) use ($storage): void {
223-
$this->postProcessEntry($storage, $path);
208+
$this->triggerPropagator($storage, $path);
224209
$this->eventDispatcher->dispatchTyped(new NodeRemovedFromCache($storage, $path));
225210
});
226211
$scanner->listen('\OC\Files\Cache\Scanner', 'updateCache', function ($path) use ($storage): void {
227-
$this->postProcessEntry($storage, $path);
212+
$this->triggerPropagator($storage, $path);
228213
$this->eventDispatcher->dispatchTyped(new FileCacheUpdated($storage, $path));
229214
});
230215
$scanner->listen('\OC\Files\Cache\Scanner', 'addToCache', function ($path, $storageId, $data, $fileId) use ($storage): void {
231-
$this->postProcessEntry($storage, $path);
216+
$this->triggerPropagator($storage, $path);
232217
if ($fileId) {
233218
$this->eventDispatcher->dispatchTyped(new FileCacheUpdated($storage, $path));
234219
} else {
@@ -240,9 +225,6 @@ public function scan(string $dir = '', $recursive = \OC\Files\Cache\Scanner::SCA
240225
throw new NotFoundException($dir);
241226
}
242227

243-
if ($this->useTransaction) {
244-
$this->db->beginTransaction();
245-
}
246228
try {
247229
$propagator = $storage->getPropagator();
248230
$propagator->beginBatch();
@@ -265,28 +247,10 @@ public function scan(string $dir = '', $recursive = \OC\Files\Cache\Scanner::SCA
265247
$this->logger->error('Storage ' . $storage->getId() . ' not available', ['exception' => $e]);
266248
$this->emit('\OC\Files\Utils\Scanner', 'StorageNotAvailable', [$e]);
267249
}
268-
if ($this->useTransaction) {
269-
$this->db->commit();
270-
}
271250
}
272251
}
273252

274253
private function triggerPropagator(IStorage $storage, $internalPath) {
275254
$storage->getPropagator()->propagateChange($internalPath, time());
276255
}
277-
278-
private function postProcessEntry(IStorage $storage, $internalPath) {
279-
$this->triggerPropagator($storage, $internalPath);
280-
if ($this->useTransaction) {
281-
$this->entriesToCommit++;
282-
if ($this->entriesToCommit >= self::MAX_ENTRIES_TO_COMMIT) {
283-
$propagator = $storage->getPropagator();
284-
$this->entriesToCommit = 0;
285-
$this->db->commit();
286-
$propagator->commitBatch();
287-
$this->db->beginTransaction();
288-
$propagator->beginBatch();
289-
}
290-
}
291-
}
292256
}

‎tests/lib/Files/EtagTest.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
namespace Test\Files;
1010

1111
use OC\Files\Filesystem;
12+
use OC\Files\SetupManager;
1213
use OC\Files\Utils\Scanner;
1314
use OCA\Files_Sharing\AppInfo\Application;
1415
use OCP\EventDispatcher\IEventDispatcher;
@@ -73,7 +74,14 @@ public function testNewUser(): void {
7374
$files = ['/foo.txt', '/folder/bar.txt', '/folder/subfolder', '/folder/subfolder/qwerty.txt'];
7475
$originalEtags = $this->getEtags($files);
7576

76-
$scanner = new Scanner($user1, Server::get(IDBConnection::class), Server::get(IEventDispatcher::class), Server::get(LoggerInterface::class));
77+
$scanner = new Scanner(
78+
$user1,
79+
Server::get(IDBConnection::class),
80+
Server::get(IEventDispatcher::class),
81+
Server::get(LoggerInterface::class),
82+
Server::get(SetupManager::class),
83+
Server::get(IUserManager::class),
84+
);
7785
$scanner->backgroundScan('/');
7886

7987
$newEtags = $this->getEtags($files);

‎tests/lib/Files/Utils/ScannerTest.php‎

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
use OC\Files\Filesystem;
1212
use OC\Files\Mount\MountPoint;
13+
use OC\Files\SetupManager;
1314
use OC\Files\Storage\Temporary;
1415
use OC\Files\Utils\Scanner;
1516
use OCP\EventDispatcher\IEventDispatcher;
@@ -77,7 +78,14 @@ public function testReuseExistingRoot(): void {
7778
$storage->file_put_contents('foo.txt', 'qwerty');
7879
$storage->file_put_contents('folder/bar.txt', 'qwerty');
7980

80-
$scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class));
81+
$scanner = new TestScanner(
82+
'',
83+
Server::get(IDBConnection::class),
84+
$this->createMock(IEventDispatcher::class),
85+
Server::get(LoggerInterface::class),
86+
Server::get(SetupManager::class),
87+
Server::get(IUserManager::class),
88+
);
8189
$scanner->addMount($mount);
8290

8391
$scanner->scan('');
@@ -99,7 +107,14 @@ public function testReuseExistingFile(): void {
99107
$storage->file_put_contents('foo.txt', 'qwerty');
100108
$storage->file_put_contents('folder/bar.txt', 'qwerty');
101109

102-
$scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class));
110+
$scanner = new TestScanner(
111+
'',
112+
Server::get(IDBConnection::class),
113+
$this->createMock(IEventDispatcher::class),
114+
Server::get(LoggerInterface::class),
115+
Server::get(SetupManager::class),
116+
Server::get(IUserManager::class),
117+
);
103118
$scanner->addMount($mount);
104119

105120
$scanner->scan('');
@@ -137,7 +152,14 @@ public function testScanSubMount(): void {
137152
$storage->file_put_contents('foo.txt', 'qwerty');
138153
$storage->file_put_contents('folder/bar.txt', 'qwerty');
139154

140-
$scanner = new Scanner($uid, Server::get(IDBConnection::class), Server::get(IEventDispatcher::class), Server::get(LoggerInterface::class));
155+
$scanner = new Scanner(
156+
$uid,
157+
Server::get(IDBConnection::class),
158+
Server::get(IEventDispatcher::class),
159+
Server::get(LoggerInterface::class),
160+
Server::get(SetupManager::class),
161+
Server::get(IUserManager::class),
162+
);
141163

142164
$this->assertFalse($cache->inCache('folder/bar.txt'));
143165
$scanner->scan('/' . $uid . '/files/foo');
@@ -166,7 +188,14 @@ public function testInvalidPathScanning($invalidPath): void {
166188
$this->expectException(\InvalidArgumentException::class);
167189
$this->expectExceptionMessage('Invalid path to scan');
168190

169-
$scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class));
191+
$scanner = new TestScanner(
192+
'',
193+
Server::get(IDBConnection::class),
194+
$this->createMock(IEventDispatcher::class),
195+
Server::get(LoggerInterface::class),
196+
Server::get(SetupManager::class),
197+
Server::get(IUserManager::class),
198+
);
170199
$scanner->scan($invalidPath);
171200
}
172201

@@ -180,7 +209,14 @@ public function testPropagateEtag(): void {
180209
$storage->file_put_contents('folder/bar.txt', 'qwerty');
181210
$storage->touch('folder/bar.txt', time() - 200);
182211

183-
$scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class));
212+
$scanner = new TestScanner(
213+
'',
214+
Server::get(IDBConnection::class),
215+
$this->createMock(IEventDispatcher::class),
216+
Server::get(LoggerInterface::class),
217+
Server::get(SetupManager::class),
218+
Server::get(IUserManager::class),
219+
);
184220
$scanner->addMount($mount);
185221

186222
$scanner->scan('');
@@ -206,7 +242,14 @@ public function testShallow(): void {
206242
$storage->file_put_contents('folder/bar.txt', 'qwerty');
207243
$storage->file_put_contents('folder/subfolder/foobar.txt', 'qwerty');
208244

209-
$scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class));
245+
$scanner = new TestScanner(
246+
'',
247+
Server::get(IDBConnection::class),
248+
$this->createMock(IEventDispatcher::class),
249+
Server::get(LoggerInterface::class),
250+
Server::get(SetupManager::class),
251+
Server::get(IUserManager::class),
252+
);
210253
$scanner->addMount($mount);
211254

212255
$scanner->scan('', $recusive = false);

0 commit comments

Comments
 (0)