Skip to content

Commit c69e5d9

Browse files
artongebackportbot[bot]
artonge
authored and
backportbot[bot]
committed
feat(files_trashbin): Refactor expire background job to support parallel run
- Follow-up of #51600 The original PR introduced the possibility to continue an `ExpireTrash` job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed. But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster. This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit. Signed-off-by: Louis Chemineau <louis@chmn.me> [skip ci]
1 parent bf18ec7 commit c69e5d9

File tree

2 files changed

+109
-37
lines changed

2 files changed

+109
-37
lines changed

‎apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php‎

Lines changed: 79 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,32 +6,42 @@
66
*/
77
namespace OCA\Files_Trashbin\BackgroundJob;
88

9+
use OC\Files\SetupManager;
910
use OC\Files\View;
11+
use OCA\Files_Trashbin\AppInfo\Application;
1012
use OCA\Files_Trashbin\Expiration;
1113
use OCA\Files_Trashbin\Helper;
1214
use OCA\Files_Trashbin\Trashbin;
1315
use OCP\AppFramework\Utility\ITimeFactory;
1416
use OCP\BackgroundJob\TimedJob;
1517
use OCP\IAppConfig;
18+
use OCP\IUser;
1619
use OCP\IUserManager;
20+
use OCP\Lock\ILockingProvider;
1721
use Psr\Log\LoggerInterface;
1822

1923
class ExpireTrash extends TimedJob {
24+
public const TOGGLE_CONFIG_KEY_NAME = 'background_job_expire_trash';
25+
public const OFFSET_CONFIG_KEY_NAME = 'background_job_expire_trash_offset';
26+
private const THIRTY_MINUTES = 30 * 60;
27+
private const USER_BATCH_SIZE = 10;
28+
2029
public function __construct(
2130
private IAppConfig $appConfig,
2231
private IUserManager $userManager,
2332
private Expiration $expiration,
2433
private LoggerInterface $logger,
34+
private SetupManager $setupManager,
35+
private ILockingProvider $lockingProvider,
2536
ITimeFactory $time,
2637
) {
2738
parent::__construct($time);
28-
// Run once per 30 minutes
29-
$this->setInterval(60 * 30);
39+
$this->setInterval(self::THIRTY_MINUTES);
3040
}
3141

3242
protected function run($argument) {
33-
$backgroundJob = $this->appConfig->getValueString('files_trashbin', 'background_job_expire_trash', 'yes');
34-
if ($backgroundJob === 'no') {
43+
$backgroundJob = $this->appConfig->getValueBool(Application::APP_ID, self::TOGGLE_CONFIG_KEY_NAME, true);
44+
if (!$backgroundJob) {
3545
return;
3646
}
3747

@@ -40,48 +50,89 @@ protected function run($argument) {
4050
return;
4151
}
4252

43-
$stopTime = time() + 60 * 30; // Stops after 30 minutes.
44-
$offset = $this->appConfig->getValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
45-
$users = $this->userManager->getSeenUsers($offset);
53+
$startTime = time();
4654

47-
foreach ($users as $user) {
48-
try {
55+
// Process users in batches of 10, but don't run for more than 30 minutes
56+
while (time() < $startTime + self::THIRTY_MINUTES) {
57+
$offset = $this->getNextOffset();
58+
$users = $this->userManager->getSeenUsers($offset, self::USER_BATCH_SIZE);
59+
$count = 0;
60+
61+
foreach ($users as $user) {
4962
$uid = $user->getUID();
50-
if (!$this->setupFS($uid)) {
51-
continue;
63+
$count++;
64+
65+
try {
66+
if ($this->setupFS($user)) {
67+
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
68+
Trashbin::deleteExpiredFiles($dirContent, $uid);
69+
}
70+
} catch (\Throwable $e) {
71+
$this->logger->error('Error while expiring trashbin for user ' . $uid, ['exception' => $e]);
72+
} finally {
73+
$this->setupManager->tearDown();
5274
}
53-
$dirContent = Helper::getTrashFiles('/', $uid, 'mtime');
54-
Trashbin::deleteExpiredFiles($dirContent, $uid);
55-
} catch (\Throwable $e) {
56-
$this->logger->error('Error while expiring trashbin for user ' . $user->getUID(), ['exception' => $e]);
5775
}
5876

59-
$offset++;
60-
61-
if ($stopTime < time()) {
62-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', $offset);
63-
\OC_Util::tearDownFS();
64-
return;
77+
// If the last batch was not full it means that we reached the end of the user list.
78+
if ($count < self::USER_BATCH_SIZE) {
79+
$this->resetOffset();
6580
}
6681
}
67-
68-
$this->appConfig->setValueInt('files_trashbin', 'background_job_expire_trash_offset', 0);
69-
\OC_Util::tearDownFS();
7082
}
7183

7284
/**
7385
* Act on behalf on trash item owner
7486
*/
75-
protected function setupFS(string $user): bool {
76-
\OC_Util::tearDownFS();
77-
\OC_Util::setupFS($user);
87+
protected function setupFS(IUser $user): bool {
88+
$this->setupManager->setupForUser($user);
7889

7990
// Check if this user has a trashbin directory
80-
$view = new View('/' . $user);
91+
$view = new View('/' . $user->getUID());
8192
if (!$view->is_dir('/files_trashbin/files')) {
8293
return false;
8394
}
8495

8596
return true;
8697
}
98+
99+
private function getNextOffset(): int {
100+
return $this->runMutexOperation(function () {
101+
$this->appConfig->clearCache();
102+
103+
$offset = $this->appConfig->getValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0);
104+
$this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, $offset + self::USER_BATCH_SIZE);
105+
106+
return $offset;
107+
});
108+
109+
}
110+
111+
private function resetOffset() {
112+
$this->runMutexOperation(function () {
113+
$this->appConfig->setValueInt(Application::APP_ID, self::OFFSET_CONFIG_KEY_NAME, 0);
114+
});
115+
}
116+
117+
private function runMutexOperation($operation): mixed {
118+
$acquired = false;
119+
120+
while ($acquired === false) {
121+
try {
122+
$this->lockingProvider->acquireLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE, 'Expire trashbin background job offset');
123+
$acquired = true;
124+
} catch (\OCP\Lock\LockedException $e) {
125+
// wait a bit and try again
126+
usleep(100000);
127+
}
128+
}
129+
130+
try {
131+
$result = $operation();
132+
} finally {
133+
$this->lockingProvider->releaseLock(self::OFFSET_CONFIG_KEY_NAME, ILockingProvider::LOCK_EXCLUSIVE);
134+
}
135+
136+
return $result;
137+
}
87138
}

‎apps/files_trashbin/tests/BackgroundJob/ExpireTrashTest.php‎

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,15 @@
77

88
namespace OCA\Files_Trashbin\Tests\BackgroundJob;
99

10+
use OC\Files\SetupManager;
11+
use OCA\Files_Trashbin\AppInfo\Application;
1012
use OCA\Files_Trashbin\BackgroundJob\ExpireTrash;
1113
use OCA\Files_Trashbin\Expiration;
1214
use OCP\AppFramework\Utility\ITimeFactory;
1315
use OCP\BackgroundJob\IJobList;
1416
use OCP\IAppConfig;
1517
use OCP\IUserManager;
18+
use OCP\Lock\ILockingProvider;
1619
use PHPUnit\Framework\MockObject\MockObject;
1720
use Psr\Log\LoggerInterface;
1821
use Test\TestCase;
@@ -44,6 +47,8 @@ protected function setUp(): void {
4447
$this->expiration = $this->createMock(Expiration::class);
4548
$this->jobList = $this->createMock(IJobList::class);
4649
$this->logger = $this->createMock(LoggerInterface::class);
50+
$this->setupManager = $this->createMock(SetupManager::class);
51+
$this->lockingProvider = $this->createMock(ILockingProvider::class);
4752

4853
$this->time = $this->createMock(ITimeFactory::class);
4954
$this->time->method('getTime')
@@ -56,25 +61,41 @@ protected function setUp(): void {
5661
}
5762

5863
public function testConstructAndRun(): void {
59-
$this->appConfig->method('getValueString')
60-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
61-
->willReturn('yes');
64+
$this->appConfig->method('getValueBool')
65+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
66+
->willReturn(true);
6267
$this->appConfig->method('getValueInt')
63-
->with('files_trashbin', 'background_job_expire_trash_offset', 0)
68+
->with(Application::APP_ID, ExpireTrash::OFFSET_CONFIG_KEY_NAME, 0)
6469
->willReturn(0);
6570

66-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
71+
$job = new ExpireTrash(
72+
$this->appConfig,
73+
$this->userManager,
74+
$this->expiration,
75+
$this->logger,
76+
$this->setupManager,
77+
$this->lockingProvider,
78+
$this->time,
79+
);
6780
$job->start($this->jobList);
6881
}
6982

7083
public function testBackgroundJobDeactivated(): void {
71-
$this->appConfig->method('getValueString')
72-
->with('files_trashbin', 'background_job_expire_trash', 'yes')
73-
->willReturn('no');
84+
$this->appConfig->method('getValueBool')
85+
->with(Application::APP_ID, ExpireTrash::TOGGLE_CONFIG_KEY_NAME, true)
86+
->willReturn(false);
7487
$this->expiration->expects($this->never())
7588
->method('getMaxAgeAsTimestamp');
7689

77-
$job = new ExpireTrash($this->appConfig, $this->userManager, $this->expiration, $this->logger, $this->time);
90+
$job = new ExpireTrash(
91+
$this->appConfig,
92+
$this->userManager,
93+
$this->expiration,
94+
$this->logger,
95+
$this->setupManager,
96+
$this->lockingProvider,
97+
$this->time,
98+
);
7899
$job->start($this->jobList);
79100
}
80101
}

0 commit comments

Comments
 (0)